-
Notifications
You must be signed in to change notification settings - Fork 10.1k
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
Convert RefSetCache
to a proper class and to use a Map
internally
#12089
Conversation
/botio unittest |
From: Bot.io (Linux m4)ReceivedCommand cmd_unittest from @timvandermeij received. Current queue size: 0 Live output at: http://54.67.70.0:8877/b794c961b012f1c/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_unittest from @timvandermeij received. Current queue size: 0 Live output at: http://54.215.176.217:8877/2cafe516f5d10c7/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/b794c961b012f1c/output.txt Total script time: 3.70 mins
|
From: Bot.io (Windows)SuccessFull output at http://54.215.176.217:8877/2cafe516f5d10c7/output.txt Total script time: 4.91 mins
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please also unit-test the RefSetCache.has(...) === false
case, since unless I'm missing something that case isn't covered here.
Also, please run the entire test-suite just to ensure that there's no overall negative effects, performance wise, from these changes. I don't really expect there to be, given how/where the RefSetCache
is used, but just in case. (Recently I looked into using a Map
, rather than a regular Object, in Dict
and that did seem to regress some documents.)
r=me, with the above addressed; thank you!
This primitive did not have unit test coverage yet, which is important for upcoming refactoring of the primitive.
Using a `Map` instead of an `Object` provides some advantages such as cheaper ways to get the size of the cache, to find out if an entry is contained in the cache and to iterate over the cache. Moreover, we can clear and re-use the same `Map` object now instead of creating a new one.
dc318f2
to
b19a179
Compare
/botio test |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @timvandermeij received. Current queue size: 0 Live output at: http://54.67.70.0:8877/37df9925a935bac/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @timvandermeij received. Current queue size: 0 Live output at: http://54.215.176.217:8877/b583591875df139/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.67.70.0:8877/37df9925a935bac/output.txt Total script time: 26.62 mins
Image differences available at: http://54.67.70.0:8877/37df9925a935bac/reftest-analyzer.html#web=eq.log |
From: Bot.io (Windows)FailedFull output at http://54.215.176.217:8877/b583591875df139/output.txt Total script time: 28.53 mins
Image differences available at: http://54.215.176.217:8877/b583591875df139/reftest-analyzer.html#web=eq.log |
This is a follow-up on #11977 (comment).