Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

When aggregating reports, inflate one expanded datatable at a time #17817

Merged
merged 26 commits into from Aug 19, 2021
Merged

When aggregating reports, inflate one expanded datatable at a time #17817

merged 26 commits into from Aug 19, 2021

Conversation

diosmosis
Copy link
Member

Description:

Currently when aggregating reports over multi periods, we get the blob data for every child period, inflate the blob data for every period into a DataTable tree, then add them together. Afterwards we clean up the extra DataTables, but in the meantime a lot of memory is used, especially when the reports are large. This PR switches to inflating the DataTable tree for just one period at a time and should use less memory overall.

Review

  • Functional review done
  • Potential edge cases thought about (behavior of the code with strange input, with strange internal state or possible interactions with other Matomo subsystems)
  • Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • Security review done see checklist
  • Code review done
  • Tests were added if useful/possible
  • Reviewed for breaking changes
  • Developer changelog updated if needed
  • Documentation added if needed
  • Existing documentation updated if needed

@diosmosis
datatable should always be a map here
dd9211d
@diosmosis
rewrite datatable aggregation to inflate a single expanded table at a…
5975626 
… time, rather than all at once
@diosmosis
when adding a new row, make sure we do not share row instances betwee…
8d2e2de 
…n datatables (as this will mean sharing subtable instances as well)
@diosmosis
support deleting datatables between specific range of IDs
a0ca4af
@diosmosis
fix datatable aggregation code that expands one datatable at a time
cc38163
@diosmosis
fix parameter issue
2f6cae3
@diosmosis
Merge branch '4.x-dev' into l3-126-datatable-single-blob
d740d61
@diosmosis
fix one type of test failure
7b6b7b0
@diosmosis
undo last change, variation in precision is expected in column values
8ca6116
@diosmosis
remove plugin submodule update
73dc2ff
@diosmosis
debugging change
33c97dd
@diosmosis
change
dae337b
@diosmosis
debug changes
2183d02
@diosmosis
undo some debug changes, metadata changes are expected
d3c04bf
@diosmosis
Merge branch '4.x-dev' into l3-126-datatable-single-blob
00b6187
@diosmosis
small refactor
a1ed7ff
@diosmosis
decimal and actions metadata changes expected
f5f005b
@diosmosis
lat long changes are for duplicates, expected
ccc2cd7
@diosmosis
correctly set metadata on created table
8236f16
@diosmosis
fix test
bdfcc7a
@diosmosis
Copy link
Member Author

Transitions screenshot change is not correct, but it is due to an unrelated bug: #17875

@diosmosis diosmosis added the Needs Review PRs that need a code review label Aug 17, 2021
@diosmosis diosmosis added this to the 4.5.0 milestone Aug 17, 2021
@diosmosis diosmosis requested a review from sgiehl August 17, 2021 23:38
@diosmosis
Copy link
Member Author

@sgiehl removed the debugging code and fixed some tests

sgiehl
sgiehl approved these changes Aug 19, 2021
View reviewed changes
Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Tests seem to be passing again 👍
Not sure if maybe @tsteur wants to have another quick look before merging?

@tsteur
Copy link
Member

tsteur commented Aug 19, 2021

Looks good to me @diosmosis 👍 Great, this will reduce memory quite a bit 💯

@diosmosis diosmosis merged commit 18d04c1 into 4.x-dev Aug 19, 2021
@diosmosis diosmosis deleted the l3-126-datatable-single-blob branch August 19, 2021 22:01
@tsteur tsteur mentioned this pull request Sep 6, 2021
Closed
@tsteur tsteur mentioned this pull request Nov 11, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sgiehl sgiehl sgiehl approved these changes

Assignees
No one assigned
Labels
Needs Review PRs that need a code review
Projects
None yet
Milestone
4.5.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@diosmosis @tsteur @sgiehl