Skip to content

don't use mmap for compression except for huge chunks in V4 raw index #7931

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.

Sign up for GitHub

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
Merged
siddharthteotia merged 1 commit into from
Dec 21, 2021
Merged

don't use mmap for compression except for huge chunks in V4 raw index #7931

siddharthteotia merged 1 commit into from
Dec 21, 2021

Conversation

@richardstartin
Copy link
Member

@richardstartin richardstartin commented Dec 19, 2021 •
edited
Loading

Following on from #7930, it's possible to speed up building of V4 raw indexes slightly (~10%) by using a compression buffer and only using memory mapping for huge chunks. The size of the compression buffer is fixed because the size of the chunk buffer itself is fixed, so the potential for very large compression buffers which exists with V2 and V3 does not apply here.

master

Benchmark                                  (_chunkCompressionType)      (_distribution)  (_maxChunkSize)  (_records)  Mode  Cnt           Score     Error  Units
BenchmarkRawForwardIndexWriter.writeV4                         LZ4  UNIFORM(1000,10000)          1048576      100000    ss    5        9857.203 ± 167.084  ms/op
BenchmarkRawForwardIndexWriter.writeV4:b                       LZ4  UNIFORM(1000,10000)          1048576      100000    ss    5  7781048545.000                #
BenchmarkRawForwardIndexWriter.writeV4:kb                      LZ4  UNIFORM(1000,10000)          1048576      100000    ss    5     7598680.000                #
BenchmarkRawForwardIndexWriter.writeV4:mb                      LZ4  UNIFORM(1000,10000)          1048576      100000    ss    5        7420.000                #
BenchmarkRawForwardIndexWriter.writeV4                         LZ4           EXP(0.001)          1048576      100000    ss    5        1802.487 ± 112.871  ms/op
BenchmarkRawForwardIndexWriter.writeV4:b                       LZ4           EXP(0.001)          1048576      100000    ss    5  1416542875.000                #
BenchmarkRawForwardIndexWriter.writeV4:kb                      LZ4           EXP(0.001)          1048576      100000    ss    5     1383340.000                #
BenchmarkRawForwardIndexWriter.writeV4:mb                      LZ4           EXP(0.001)          1048576      100000    ss    5        1350.000                #

branch

Benchmark                                  (_chunkCompressionType)      (_distribution)  (_maxChunkSize)  (_records)  Mode  Cnt           Score     Error  Units
BenchmarkRawForwardIndexWriter.writeV4                         LZ4  UNIFORM(1000,10000)          1048576      100000    ss    5        8788.638 ± 256.238  ms/op
BenchmarkRawForwardIndexWriter.writeV4:b                       LZ4  UNIFORM(1000,10000)          1048576      100000    ss    5  7781048545.000                #
BenchmarkRawForwardIndexWriter.writeV4:kb                      LZ4  UNIFORM(1000,10000)          1048576      100000    ss    5     7598680.000                #
BenchmarkRawForwardIndexWriter.writeV4:mb                      LZ4  UNIFORM(1000,10000)          1048576      100000    ss    5        7420.000                #
BenchmarkRawForwardIndexWriter.writeV4                         LZ4           EXP(0.001)          1048576      100000    ss    5        1625.317 ±  48.117  ms/op
BenchmarkRawForwardIndexWriter.writeV4:b                       LZ4           EXP(0.001)          1048576      100000    ss    5  1416542875.000                #
BenchmarkRawForwardIndexWriter.writeV4:kb                      LZ4           EXP(0.001)          1048576      100000    ss    5     1383340.000                #
BenchmarkRawForwardIndexWriter.writeV4:mb                      LZ4           EXP(0.001)          1048576      100000    ss    5        1350.000                #

@richardstartin richardstartin changed the title don't use mmap for compression except for huge chunks don't use mmap for compression except for huge chunks in V4 raw index Dec 19, 2021
@codecov-commenter
Copy link

codecov-commenter commented Dec 19, 2021 •
edited
Loading

Codecov Report

Merging #7931 (319bd58) into master (6037cac) will decrease coverage by 1.18%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #7931      +/-   ##
============================================
- Coverage     71.15%   69.97%   -1.19%     
- Complexity     4111     4118       +7     
============================================
  Files          1593     1593              
  Lines         82365    82377      +12     
  Branches      12270    12273       +3     
============================================
- Hits          58609    57642     -967     
- Misses        19806    20799     +993     
+ Partials       3950     3936      -14
Flag Coverage Δ
integration1 ?
integration2 27.66% <0.00%> (+0.07%) ⬆️
unittests1 68.09% <100.00%> (+0.02%) ⬆️
unittests2 14.32% <0.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...riter/impl/VarByteChunkSVForwardIndexWriterV4.java 96.69% <100.00%> (+0.36%) ⬆️
...pinot/minion/exception/TaskCancelledException.java 0.00% <0.00%> (-100.00%) ⬇️
...nverttorawindex/ConvertToRawIndexTaskExecutor.java 0.00% <0.00%> (-100.00%) ⬇️
...e/pinot/common/minion/MergeRollupTaskMetadata.java 0.00% <0.00%> (-94.74%) ⬇️
...plugin/segmentuploader/SegmentUploaderDefault.java 0.00% <0.00%> (-87.10%) ⬇️
.../transform/function/MapValueTransformFunction.java 0.00% <0.00%> (-85.30%) ⬇️
...ot/common/messages/RoutingTableRebuildMessage.java 0.00% <0.00%> (-81.82%) ⬇️
...data/manager/realtime/DefaultSegmentCommitter.java 0.00% <0.00%> (-80.00%) ⬇️
...ache/pinot/common/lineage/SegmentLineageUtils.java 22.22% <0.00%> (-77.78%) ⬇️
...ore/startree/executor/StarTreeGroupByExecutor.java 0.00% <0.00%> (-77.78%) ⬇️
... and 101 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6037cac...319bd58. Read the comment docs.

@siddharthteotia
Copy link
Contributor

siddharthteotia commented Dec 20, 2021 •
edited
Loading

@richardstartin , can we please have a short discussion around raw index. I didn't get a chance to review v4 format and want to be on the same page here w.r.t changes / improvements being made.

Want to also make sure that V4 subsumes the V3 format changes I had introduced since V3 supports a critical production use case of ours. It is configurable through FieldConfig. I wanted to make V3 default going forward but looks like you want to make V4 default.

So, let's please have a short discussion to get clarity on the path forward. I can then help review this and PR #7934

Will follow-up offline on the discussion.

@mcvsubbu
Copy link
Contributor

mcvsubbu commented Dec 21, 2021

Thanks, @siddharthteotia

@siddharthteotia
Copy link
Contributor

siddharthteotia commented Dec 21, 2021 •
edited
Loading

Discussed offline with @richardstartin

  • The change here is limited to V4 format and I am ok with the reason behind the change. Basically, with V4 the use of memory mapped buffer for compression is pretty much eliminated. So the direct buffer is introduced in this PR to help improve the performance for the vast majority of cases

  • Regarding V3 v/s V4: they have similarities but the main difference is 4GB limitation imposed in V4. V3 has no such limitation as the chunk offset in the file header is tracked using long. V3 also derives the numDocsPerChunk (as opposed to using fixed 1000 rows per chunk) or packs a single row in the chunk if the row > 1MB. In V3 we never thought of using memory mapping for compressed buffer even though our chunks are potentially huge in size (from the production use case where V3 is being used).

  • Regarding changing the default from V2: I didn't make V3 default initially since wanted to test it out in production. It's been used for over an year now and hasn't had any issues. We discussed that we can bridge the functionality gap between V3 and V4 as a new change and then hopefully remove V3 code (ensuring the V4+ reader continues to read it) if possible since V3 and V4 had common goals

@mcvsubbu
Copy link
Contributor

mcvsubbu commented Dec 21, 2021

Thanks for reaching consensus, we should do more of these interactions. Way faster than comments in PRs.

@richardstartin
Copy link
Member Author

richardstartin commented Dec 21, 2021

For posterity, feedback was solicited on the design proposal for the V4 index in #7616.

@siddharthteotia siddharthteotia merged commit 0eb63c5 into apache:master Dec 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@siddharthteotia siddharthteotia siddharthteotia approved these changes

@Jackie-Jiang Jackie-Jiang Jackie-Jiang approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@richardstartin @codecov-commenter @siddharthteotia @mcvsubbu @Jackie-Jiang