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.

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

LUCENE-10499: reduce unnecessary copy data overhead when growing array size #786

Merged
merged 5 commits into from
Apr 26, 2022
Merged

LUCENE-10499: reduce unnecessary copy data overhead when growing array size #786

merged 5 commits into from
Apr 26, 2022

Conversation

wjp719
Copy link
Contributor

@wjp719 wjp719 commented Apr 5, 2022 •
edited
Loading

ArrayUtil#grow will copy origin array data to the new array after growing size in default, but in many places, the new array needn't the origin data. This PR reduces the extra copy data overhead.

Checklist

Please review the following and check all that apply:

  • I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
  • I have created a Jira issue and added the issue ID to my pull request title.
  • I have given Lucene maintainers access to contribute to my PR branch. (optional but recommended)
  • I have developed this patch against the main branch.
  • I have run ./gradlew check.
  • I have added tests for my changes.

@wjp719 wjp719 changed the title LUCENE-10499: eliminate unnecessary copy data overhead when growing array size LUCENE-10499: reduce unnecessary copy data overhead when growing array size Apr 5, 2022
@rmuir
Copy link
Member

rmuir commented Apr 5, 2022

Can we instead add a new method to ArrayUtil, with a different name than grow, that doesn't copy data?

I don't think we should add a boolean parameter to grow that gives it radically different behavior.

@wjp719
Copy link
Contributor Author

wjp719 commented Apr 5, 2022 •
edited
Loading

Can we instead add a new method to ArrayUtil, with a different name than grow, that doesn't copy data?

I don't think we should add a boolean parameter to grow that gives it radically different behavior.

@rmuir Hi, I add ArrayUtil#growSizeOnly method that only grow size and not copy array data, please review again, thanks.

@wjp719
Copy link
Contributor Author

wjp719 commented Apr 6, 2022

@jpountz Hi, can you help to take some time to review this PR, thanks a lot

jpountz
jpountz approved these changes Apr 6, 2022
View reviewed changes
Copy link
Contributor

@jpountz jpountz left a comment

Choose a reason for hiding this comment

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

Thanks, the change looks correct to me. I'm not a fan of the new method's name, but I don't have a better suggestion. I'll merge this change in a few days unless someone objects.

@msokolov
Copy link
Contributor

msokolov commented Apr 6, 2022

I don't much like the name either. I wouldn't block, but perhaps growWithoutCopying? or growNoCopy? The whole idea that we are growing the existing array is deceptive though because really we are just creating a new array (maybe), but I can't improve on the name much either,

@wjp719
Copy link
Contributor Author

wjp719 commented Apr 7, 2022

@msokolov @jpountz Thank you for your comments, I refactored method name as ArrayUtil#growNoCopy.

@wjp719 wjp719 requested a review from jpountz April 7, 2022 02:24
@wjp719
Copy link
Contributor Author

wjp719 commented Apr 18, 2022

Thanks, the change looks correct to me. I'm not a fan of the new method's name, but I don't have a better suggestion. I'll merge this change in a few days unless someone objects.

@jpountz Hi, there are no more reviews for a few days, can you help to merge this pr? thanks.

@wjp719
Copy link
Contributor Author

wjp719 commented Apr 21, 2022

@rmuir @jpountz Hi, this pr is ready to be merged, thanks.

@jpountz jpountz merged commit ebe2d7b into apache:main Apr 26, 2022
jpountz pushed a commit that referenced this pull request Apr 26, 2022
@wjp719 @jpountz
LUCENE-10499: reduce unnecessary copy data overhead when growing arra…
9abec89 
…y size (#786)

Co-authored-by: xiaoping.wjp <xiaoping.wjp@alibaba-inc.com>
@wjp719 wjp719 deleted the feature/avoid_grow_copy branch April 27, 2022 09:11
wjp719 added a commit to wjp719/lucene that referenced this pull request Apr 28, 2022
@wjp719
Merge branch 'main' into feature/skip_useless_attemps
3763818 
* main: (68 commits)
  LUCENE-10292: Suggest: Fix FreeTextSuggester so that getCount() returned results consistent with lookup() during concurrent build()
  LUCENE-10525 Improve WindowsFS emulation to catch invalid file names (apache#829)
  LUCENE-10508:  Use MIN_WIDE_EXTENT for all wide rectangles (apache#845)
  LUCENE-10470: [Tessellator] Fix some failing polygons due to collinear edges (apache#756)
  LUCENE-10508: Fix error for rectangles with an extent close to 180 degrees (apache#824)
  LUCENE-10529: Fix TestTaxonomyFacetAssociations NPE when randomly indexing no documents for dim
  fix path to jar file in demo documentation
  LUCENE-10499: reduce unnecessary copy data overhead when growing array size (apache#786)
  LUCENE-10535: upgrade com.palantir.consistent-versions to 2.10.0
  LUCENE-10533: SpellChecker.formGrams is missing bounds check (apache#836)
  Upgrade spotless and use runToFixMessage for 'gradlew tidy' hint. (apache#834)
  Fix JVM error branch logic. (apache#835)
  LUCENE-10493: factor out Viterbi algorithm and share it between kuromoji and nori (apache#805)
  LUCENE-8836: Speed up TermsEnum#lookupOrd on increasing sequences of ords. (apache#827)
  LUCENE-10528: use Xvfb in test to avoid messing up user's desktop (apache#828)
  LUCENE-10315: Speed up DocIdsWriter by ForUtil (apache#797)
  LUCENE-10517: Improve performance of SortedSetDV faceting by iterating on class types (apache#812)
  Add two facet tests (apache#826)
  fail clearly on too-new JDK (apache#819)
  improve spotless error to suggest running 'gradlew tidy' (apache#817)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jpountz jpountz Awaiting requested review from jpountz

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@wjp719 @rmuir @msokolov @jpountz