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.

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

Ispn 3023 #3265

Closed
wants to merge 2 commits into from

Conversation

@wburns
Copy link
Member

commented Feb 20, 2015

@wburns

This comment has been minimized.

Copy link
Member Author

commented Feb 23, 2015

Closing for now, I found an issue with LIRS that still needs fixing.

@wburns wburns closed this Feb 23, 2015

@wburns

This comment has been minimized.

Copy link
Member Author

commented Feb 25, 2015

Okay there is only 1 known issue remaining that I haven't figured out yet. Basically if seems if you are evicting a value and it is removed at the same time it can cause a miscount for LIR residents (I haven't been able to reproduce with tracing yet to confirm)

However, good news is with the refactoring the LIRS implementation is only about 10-15% slower than LRU (actually faster than the old LRU)

@wburns wburns reopened this Feb 25, 2015

@wburns

This comment has been minimized.

Copy link
Member Author

commented Feb 25, 2015

I will squash the commits when I resolve the remaining issue. Ignore any kind of trace messages still left as they will be removed before final commit.

@wburns wburns added the Preview label Feb 25, 2015

@wburns wburns force-pushed the wburns:ISPN-3023 branch from 4aa3b22 to c44fae6 Feb 25, 2015

@wburns

This comment has been minimized.

Copy link
Member Author

commented Feb 25, 2015

Remaining issues with concurrent removes should now be fixed. Code was erroneously assuming when it saw a removed value from LIR demotion that it didn't need to get another LIR.

I am still testing some more, but the code appears to be working from the extensive testing I have done. Note please look at MapStressTest for the best testing changes.

@wburns wburns force-pushed the wburns:ISPN-3023 branch 3 times, most recently from 880fa13 to bd0df0c Feb 25, 2015

@wburns

This comment has been minimized.

Copy link
Member Author

commented Feb 26, 2015

I was actually able to do a full mvn clean install successfully which hasn't happened in a long while, glad to see master is somewhat cleaned up now :)

@wburns wburns added Ready for Review and removed Preview labels Feb 26, 2015

@wburns

This comment has been minimized.

Copy link
Member Author

commented Feb 27, 2015

Looks like CI may have caught 2 test failures I didn't see, I will check those out in a bit.

@wburns

This comment has been minimized.

Copy link
Member Author

commented Feb 27, 2015

I haven't been able to reproduce the failure at all. However I did notice that if the random number seed returns the same value twice in a row this could fail, since the put wouldn't be putting a new value (however this seems highly improbable to have happened twice in the same run).

I updated the test to try just a few more iterations to make sure the above mentioned issue won't cause any issues.

@wburns wburns force-pushed the wburns:ISPN-3023 branch 5 times, most recently from 3c6625f to 29f9b51 Feb 27, 2015

@wburns wburns force-pushed the wburns:ISPN-3023 branch from 29f9b51 to f73b54d Mar 11, 2015

System.out.println("Removes took: " + removeNano + " nanoseconds");
}

public void testNoEvictionRemovePerformance() {

This comment has been minimized.

Copy link
@galderz

galderz Mar 12, 2015

Member

Formatting

AnyEquivalence.INT, AnyEquivalence.INT), lru.toString());
}

@Test(priority=5)

This comment has been minimized.

Copy link
@galderz

galderz Mar 12, 2015

Member

Formatting

import static org.testng.AssertJUnit.assertFalse;

@Test(groups = "unit", testName = "persistence.decorators.AsyncStoreEvictionTest")
public class AsyncStoreEvictionTest extends AbstractInfinispanTest {

This comment has been minimized.

Copy link
@galderz

galderz Mar 12, 2015

Member

File committed by mistake?

This comment has been minimized.

Copy link
@wburns

wburns Mar 12, 2015

Author Member

Yes thanks :)

This comment has been minimized.

Copy link
@danberindei

danberindei Mar 16, 2015

Member

@wburns Forgot to push an update?

This comment has been minimized.

Copy link
@wburns

wburns Mar 16, 2015

Author Member

I removed it, sorry about that.

class StrippedConcurrentLinkedDeque<E> {
/**
* A node from which the first node on list (that is, the unique node p
* with p.prev == null && p.next != p) can be reached in O(1) time.

This comment has been minimized.

Copy link
@danberindei

danberindei Mar 16, 2015

Member

This should be in a {@code} block, so that the javadoc tools escapes the &.

This comment has been minimized.

Copy link
@wburns

wburns Mar 16, 2015

Author Member

This was how the ConcurrentLinkedDeque implementation had this. Since this variable isn't public it isn't generated in the Javadoc, so I am not sure we need to fix this.


/**
* A node from which the last node on list (that is, the unique node p
* with p.next == null && p.prev != p) can be reached in O(1) time.

This comment has been minimized.

Copy link
@danberindei

danberindei Mar 16, 2015

Member

Same here

This comment has been minimized.

Copy link
@wburns

wburns Mar 16, 2015

Author Member

This isn't generated in Javadoc since the field is package private.


/**
* Returns the first node, the unique node p for which:
* p.prev == null && p.next != p

This comment has been minimized.

Copy link
@danberindei

danberindei Mar 16, 2015

Member

And here

This comment has been minimized.

Copy link
@wburns

wburns Mar 16, 2015

Author Member

This isn't generated in Javadoc since the field is package private.


/**
* Returns the last node, the unique node p for which:
* p.next == null && p.prev != p

This comment has been minimized.

Copy link
@danberindei

danberindei Mar 16, 2015

Member

And here

This comment has been minimized.

Copy link
@wburns

wburns Mar 16, 2015

Author Member

This isn't generated in Javadoc since the field is package private.

@wburns wburns force-pushed the wburns:ISPN-3023 branch from f73b54d to e7168c2 Mar 16, 2015

@wburns

This comment has been minimized.

Copy link
Member Author

commented Mar 16, 2015

Updated to remove the extra orig file and also Javadoc isn't generated for the provided methods (it was actually like that on the base JDK class).

wburns added 2 commits Jan 16, 2015
ISPN-3023 Re-implement BoundedConcurrentHashMap using CHMv8 designs
* Added in initial commit for baseline ECHMV8
ISPN-3023 Re-implement BoundedConcurrentHashMap using CHMv8 designs
* Added new LRU & LIRS implementations

@wburns wburns force-pushed the wburns:ISPN-3023 branch from e7168c2 to 9876441 Mar 16, 2015

@danberindei

This comment has been minimized.

Copy link
Member

commented Mar 16, 2015

Integrated, thanks Will!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.