Skip to content

new OffHeapIncrementalIndex that does only aggregations off-heap#2325

Merged
drcrallen merged 4 commits intoapache:masterfrom
himanshug:gp_by_merge
Feb 5, 2016
Merged

new OffHeapIncrementalIndex that does only aggregations off-heap#2325
drcrallen merged 4 commits intoapache:masterfrom
himanshug:gp_by_merge

Conversation

@himanshug
Copy link
Copy Markdown
Contributor

Our groupBy queries contain many metric columns of type thetaSketch which are big and lead to significant GC pressure on the process, this PR enables doing metric aggregation for group by queries completely off-heap.

changes introduced:

  1. new OffHeapIncrementalIndex that is same as OnHeapIncrementalIndex but does aggregations in off-heap buffers.
  2. configuration, "useOffheap", to use OffHeapIncrementalIndex while GroupBy merging
  3. configuration, "druid.computation.buffer.pool.cacheSize", to set the upper limit on number of buffers that StupidPool can cache

fixes #2297

I understand that this PR would have conflicts with some other open PRs. I will rebase and fix merge conflicts as and when other PRs get merged.

@fjy
Copy link
Copy Markdown
Contributor

fjy commented Jan 23, 2016

@himanshug have you thought at all about extending this PR and enabling on disk computations with groupBys?

@himanshug
Copy link
Copy Markdown
Contributor Author

@fjy This is a self-sufficient step that has immediate gains without introducing any major query latency.

I'm thinking about on-disk, but it is a challenge to do that without adding significant query latency... currently, I'm in the stage of brainstorming on that in the background. That said, current PR might enable larger groupBys than before.

@nishantmonu51
Copy link
Copy Markdown
Member

@himanshug @fjy , does this need to be a blocker for 0.9.0 ?

@drcrallen
Copy link
Copy Markdown
Contributor

@himanshug Have you guys ever tried enabling swap and using more direct byte buffers than available memory?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can this just move to a protected class instead of duplicating it?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

will do

@himanshug
Copy link
Copy Markdown
Contributor Author

@drcrallen thanks for review, I will probably rework this a bit after #2085 is merged.
we have very high memory brokers and don't usually need swap at all. this PR is intended to avoid GC pauses really. at historicals, we have some subsets where more segments than available memory are loaded, but we have SSDs there and it works ok.

@himanshug
Copy link
Copy Markdown
Contributor Author

also, I do not want to block 0.9.0 RC because of this so setting the milestone for this to be 0.9.1
However, I will pull it in my internal distribution as soon as it is merged.

@himanshug himanshug modified the milestones: 0.9.1, 0.9.0 Feb 2, 2016
@himanshug himanshug force-pushed the gp_by_merge branch 4 times, most recently from 1101519 to 660f590 Compare February 3, 2016 18:54
@himanshug
Copy link
Copy Markdown
Contributor Author

@drcrallen all review comments addressed.

@fjy
Copy link
Copy Markdown
Contributor

fjy commented Feb 5, 2016

👍

Comment thread docs/content/configuration/broker.md Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That's not quite true. If I have a max size of 10 and I request 100 buffers, I'll still get 100 buffers, but I'll only return 10 for later use. Then if I request 100 I'll have 10 queued and 90 new ones. And when they are done they will return 10 and drop 90 to GC.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm at a loss right now on the proper way to document that behavior. Maybe just in the stupidPool javadoc?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes, you are right and this is just size of the cache. do you want to suggest better messaging?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

how about, "processing buffer pool caches the buffers for later use, this is the maximum count cache will grow to. however pool can create more buffers than it can cache."

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ok updated doc to be more explicit.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

FYI there's io.druid.segment.CloserRule for just this kind of use

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

that looks nicer... changed to use CloserRule.

@himanshug
Copy link
Copy Markdown
Contributor Author

@drcrallen addressed comments.

@drcrallen
Copy link
Copy Markdown
Contributor

Cool 👍 with 4 commits (after travis) @himanshug would you be willing to update this thread with performance numbers between onheap and offheap after your internal testing?

(I'm assuming that would come after this is merged)

@himanshug
Copy link
Copy Markdown
Contributor Author

@drcrallen thanks, will update with test results, thatz why I haven't made the off-heap default but opt-in.

drcrallen added a commit that referenced this pull request Feb 5, 2016
new OffHeapIncrementalIndex that does only aggregations off-heap
@drcrallen drcrallen merged commit 08802c3 into apache:master Feb 5, 2016
@himanshug himanshug deleted the gp_by_merge branch February 8, 2016 16:17
@xvrl xvrl added this to the 0.9.0 milestone Feb 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Proposal]OffHeapIncrementalIndexV2 to do just aggregations off-heap

5 participants