Convert the RefSet primitive to a proper class and use a Set internally
#11977
Conversation
|
/botio test |
There was a problem hiding this comment.
Really sorry, but I'm not at all keen on these changes!
The RefSet abstraction is, as its name suggests, written specifically to operate only on Ref objects and I worry that simply using Sets directly in this manner makes the code more difficult to read/modify and that bugs are thus much easier to introduce.
Essentially, in the worker, it's generally preferable to avoid depending directly on the stringified version of a Ref object if possible.
Basically, unless there's test-cases where benchmarking shows clear improvements, then I'd ask that we please don't make these changes.
timvandermeij
changed the title
RefSet primitiveRefSet primitive to a proper class and use a Set internally
…rnally The `RefSet` primitive predates ES6, so that most likely explains why an object is used internally to track the entries. However, nowadays we can use built-in JavaScript sets for this purpose. Built-in types are often more efficient/optimized and using it makes the code a bit more clear since we don't have to assign `true` to keys anymore just to indicate their presence.
|
I understand your concern here. I have updated the patch to keep |
|
/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/4854668430355f4/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/c20eaf2ff4ed819/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/4854668430355f4/output.txt Total script time: 3.80 mins
|
From: Bot.io (Windows)SuccessFull output at http://54.215.176.217:8877/c20eaf2ff4ed819/output.txt Total script time: 4.76 mins
|
Yes, that seems completely fine by me; thank you for doing this! (Also, possibly this could make Just for good measure, let's run all tests :-) /botio test |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/1979b1a308982ec/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.215.176.217:8877/e02fe9cb07f91ff/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.67.70.0:8877/1979b1a308982ec/output.txt Total script time: 25.59 mins
Image differences available at: http://54.67.70.0:8877/1979b1a308982ec/reftest-analyzer.html#web=eq.log |
From: Bot.io (Windows)FailedFull output at http://54.215.176.217:8877/e02fe9cb07f91ff/output.txt Total script time: 27.78 mins
Image differences available at: http://54.215.176.217:8877/e02fe9cb07f91ff/reftest-analyzer.html#web=eq.log |
|
An idea could be to try and do something similar, but obviously with a |
The
RefSetprimitive predates ES6, so that most likely explains why an object is used internally to track the entries. However, nowadays we can use built-in JavaScript sets for this purpose. Built-in types are often more efficient/optimized and using it makes the code a bit more clear since we don't have to assigntrueto keys anymore just to indicate their presence.