Skip to content

Conversation

mpeyrotc
Copy link
Member

@mpeyrotc mpeyrotc commented Sep 8, 2021

Since we want to support collections that are larger than 2^31 - 1 elements, this PR changes all our indexing logic to use longs and makes sure that in interface methods from ICollection and IReadOnlyList that require us to continue using ints we throw when we pass their max limit.

Bumps [url-parse](https://github.com/unshiftio/url-parse) from 1.5.1 to 1.5.3.
- [Release notes](https://github.com/unshiftio/url-parse/releases)
- [Commits](unshiftio/url-parse@1.5.1...1.5.3)

---
updated-dependencies:
- dependency-name: url-parse
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
@mpeyrotc
Copy link
Member Author

mpeyrotc commented Sep 8, 2021

@brianrob @sharwell this is a prerequisite to changing segmented list to support more elements. It should not have any functionality change. Could I bother you with a review?

@sharwell
Copy link
Contributor

sharwell commented Sep 8, 2021

Since we want to support collections that are larger than 231 - 1

To clarify, you are referring to the number of elements in the collection or just the number of bytes it takes to represent those elements?

@mpeyrotc
Copy link
Member Author

mpeyrotc commented Sep 8, 2021

I fixed the message, elements.

@sharwell
Copy link
Contributor

sharwell commented Sep 9, 2021

What about just making ToArray() and CopyTo() throw an exception if the collection contains more than the supported number of elements?

@mpeyrotc
Copy link
Member Author

mpeyrotc commented Sep 9, 2021

That is true, although we don't use that code at all in any of the places that use this data structure, so I am fine with either solution.

Although, we would also need to change the indexer, as well as count. That was the main reason I removed the interfaces.

@brianrob
Copy link
Member

Agreed with @sharwell, I would like to keep the implementation, but let's throw if the operation can't be run because the collection is too large.

@mpeyrotc mpeyrotc force-pushed the dev/mapeyrot/segmented_list_fix branch 3 times, most recently from 034994c to 1242006 Compare September 15, 2021 19:46
@mpeyrotc mpeyrotc changed the title No longer support Collections Interfaces in SegmentedList Let SegmentedList Handle more than 2^31-1 elements Sep 15, 2021
@brianrob brianrob changed the base branch from main to feature/large-gcdump September 17, 2021 02:48
@mpeyrotc mpeyrotc force-pushed the dev/mapeyrot/segmented_list_fix branch from 4baa765 to 7dd8b43 Compare September 21, 2021 00:17
Copy link
Member

@brianrob brianrob left a comment

Choose a reason for hiding this comment

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

Merging this into the feature branch to unblock progress. I will do further review there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants