Repository navigation
Answer then on a Flow Data without looking for Element Data - #175
Draft
jwrosewell wants to merge 2 commits into
Draft
jwrosewell wants to merge 2 commits into
jwrosewell wants to merge 2 commits into
Conversation
…led then The Pipeline resolves a promise with the Flow Data at each of its elements, and a promise reads then from what it resolves with. The Flow Data passes that read to get, which finds no element, looks again ignoring case and throws, on every request.
A promise reads then from every value it resolves with, and the Pipeline resolves one with the Flow Data at each element. The proxy passed that read to get, which since the case-ignoring lookup also scans every key before it throws, several times a request. Timed against main, that lookup cost the IP Intelligence on-premise engine about 6 per cent of its detections a second. then is never Element Data, so the proxy now answers it as undefined first, and the catch that only swallowed the error for then goes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A promise reads
thenfrom every value it resolves with, to see whether that value is itself a promise. The Pipeline resolves a promise with the Flow Data before its first element and after each one (pipeline/fiftyone.pipeline.core/pipeline.jslines 164, 176, 191, 210 and 221 at c6ba938), and the caller'sawait flowData.process()does it once more.The Flow Data is a proxy that passed each read of
thentoget(pipeline/fiftyone.pipeline.core/flowData.jslines 64 to 89).get(lines 176 to 197) logs the read at debug level, finds no element calledthen, looks again ignoring letter case, builds a message naming every key and throws it, and the proxy caught that and gaveundefined. So every request ran the lookup for a missing key several times over. Looking again ignoring letter case came with #122, which made each of those misses dearer.Timed against
mainwhilercwas checked for release, the IP Intelligence on-premise engine gave 5.8 to 8.0 per cent fewer detections a second in five runs onrccommits from 6c0b3fa to c6ba938. An earlier timing had found the loss between 7004cc6 (run 43696340276, 0.9 per cent fewer) and cdbbee8 (run 43696348092, 6.0 per cent fewer). Timing each of the five commits between them put the step at #122, with the commit before it giving 1.2 per cent more thanmainand #122's own commit 5.7 per cent fewer, as the table in Performance shows.What changed
thenasundefinedbefore any lookup, becausethenis never Element Data (flowData.jslines 71 to 78 at b337ec1). Thecatchthat existed only to swallow the error forthenis removed, so every other key behaves as before. It gives Element Data, a method or property of the Flow Data, or the error for a key no element has.fiftyone.pipeline.corechanges, so it is released, with the packages that depend on it.Before you upgrade
Nothing changes for existing code. Reading
thenfrom a Flow Data gaveundefinedbefore, through the caught error, and givesundefinednow. An element whose data key isthenis no longer found asflowData.then, which a promise would have taken for athenmethod anyway, andflowData.get('then')still finds it.Tests
A new
pipeline/fiftyone.pipeline.core/tests/flowDataThen.test.jsbuilds a Pipeline of two elements and watchesFlowData.get.thengivesundefinedand callsgetnot at all.getforthen.firstand asSECOND, throughget.rcat c6ba938 with only the test file, 0f5dd4b, and a throwaway workflow added. The first two tests fail and the third passes, as it should before and after the change. The other 584 core unit tests pass.flowData.jsand the test file.Performance
Each run below is a throwaway workflow on one Ubuntu ARM runner with Node.js 24. It times
mainat 4d5832f against one other commit with the on-premise performance console examples, Device Detection on the TAC data file and IP Intelligence on the Enterprise data file, both taken from the repository's asset cache. Each run has 21 rounds, turning the order each round so that neither goes first every time, and readsHigherIsBetter.DetectionsPerSecondfrom each example's JSON output. Figures are medians in detections a second, higher is better, with the number of rounds in which the other commit beatmain.mainmainrcrc, againmainruns the same miss forthenwithout the second look, so this head gives more detections a second thanmainas well as more thanrc. In the last run Device Detection's lowest round on this head, 40,525, is belowmain's highest, 42,644, and the same holds for IP Intelligence, 40,507 against 43,980, so the full ranges touch. The quartiles are apart, at 41,680 to 46,823 against 33,123 to 34,075 for Device Detection and 41,914 to 43,901 against 33,367 to 35,455 for IP Intelligence.Full build
Run 43752383786 on
tmp/pr-matrix-175, started bytools/pr-matrix.pywithout macOS. All 7Setting 'PullRequestSha'lines give this head, b337ec1, and Configure printed "Left out 2 macOS entries". Each of the six Build and Test jobs, Ubuntu, Ubuntu ARM and Windows on Node.js 22 and 24, passed 1,949 unit tests with 2 skipped and 124 integration tests with 4 skipped.macOS was left out of this draft's full build under the 9 November decision. The rc head is built with macOS before it merges into main.
What remains
Nothing for this change.