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

Optimize UniformSnapshot construction #970

Closed
bentatham opened this Issue Jun 28, 2016 · 2 comments

Comments

Projects
None yet
3 participants
@bentatham
Contributor

bentatham commented Jun 28, 2016

When creating a UniformSnapshot from a UniformReservoir three copies are made of the original values in the reservoir, with boxing back and forth as well. From a SlidingTimeWindowReservoir, there are two copies made.

While it somewhat "dangerous", I think it would be better in both cases to pass in a raw long[] directly to the constructor that can be used directly by the UniformSnapshot so that there is only one copy of the values, and only one exchange from Long to long.

@bentatham

This comment has been minimized.

Show comment
Hide comment
@bentatham

bentatham Jun 29, 2016

Contributor

(looking at 3.2-development) I see there is a test case UniformSnapshotTest.worksWithOverestimatedCollections. This seems like a pretty white-box test to ensure that the constructor does not rely on the size() method of the collection at all. I'm trying to understand where that case happens? Is it a concurrency issue from the ConcurrentSkipListMap values being passed in from SlidingTimeWindowReservoir? Looking at the impl of the values collection it gives for toArray, that is not thread-safe either, so I'm not sure what extra value this test provides in the real world.

Looking at commit history, this was all done as part of a big metrics-core refactor back in 2013, so no guidance given there.

Contributor

bentatham commented Jun 29, 2016

(looking at 3.2-development) I see there is a test case UniformSnapshotTest.worksWithOverestimatedCollections. This seems like a pretty white-box test to ensure that the constructor does not rely on the size() method of the collection at all. I'm trying to understand where that case happens? Is it a concurrency issue from the ConcurrentSkipListMap values being passed in from SlidingTimeWindowReservoir? Looking at the impl of the values collection it gives for toArray, that is not thread-safe either, so I'm not sure what extra value this test provides in the real world.

Looking at commit history, this was all done as part of a big metrics-core refactor back in 2013, so no guidance given there.

bentatham added a commit to bentatham/metrics that referenced this issue Jun 29, 2016

ryantenney added a commit that referenced this issue Jun 29, 2016

Merge pull request #972 from bentatham/feature/970-optimize-uniform-s…
…napshot

#970: optimize creation of UniformSnapshot

arteam added a commit that referenced this issue Mar 19, 2017

Fix creating a uniform snapshot from a collection
The current code for creating a snapshot from the collection is unfortunately broken.
The code fails if the provided collection is modified during creating of the snapshot and
it has a weak iterator (precisely what `ConcurrentSkipListMap` provides). The iterator
doesn't guarantee that it has exactly the same amount iteration as the size of the
collection. As a result, the code throws an `ArrayIndexOutOfBoundException`.
The correct behaviour is too dump the collection to a resizable array on the fly and
then convert it the array of long primitives.

The behaviour was correct in the 3.1 branch, but was broken in 3.2 by the optimization
in #970.

@arteam arteam closed this Jan 26, 2018

@storozhukBM

This comment has been minimized.

Show comment
Hide comment
@storozhukBM

storozhukBM Jan 26, 2018

Contributor

@bentatham if you're using SlidingTimeWindowReservoir try new drop-in replacement called SlidingTimeWindowArrayReservoir it should make snapshots a lot faster due to vectorized System.arraycopy usage.

Contributor

storozhukBM commented Jan 26, 2018

@bentatham if you're using SlidingTimeWindowReservoir try new drop-in replacement called SlidingTimeWindowArrayReservoir it should make snapshots a lot faster due to vectorized System.arraycopy usage.

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