Repository navigation
Put a key filter's list in lower case once, not on every check - #91
Merged
Merged
Conversation
A detection checks each piece of evidence against an engine's key filter as the evidence is added, and again as the engine reads the evidence it accepts. BasicListEvidenceKeyFilter puts every key in its list in lower case on each check, so each detection pays for the whole list at least twice. These tests count those calls, and check that letter case, an empty list, and a list changed or replaced after a check behave as before.
BasicListEvidenceKeyFilter keeps its keys in lower case in a set, made from the list on the first check and again whenever the list is replaced or changes, so a check puts only the key being checked in lower case. Which keys are kept is unchanged, and the device detection and IP Intelligence engines still pass the native engine only the evidence it reads.
One blank line after the licence header, and the private member marker where tsc writes it, after list.
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
The release draft #83 failed its performance gate on
Ubuntu_ARM_Node_24in build 43335666904, measuring 30,352 detections a second against an average of 33,900 and a floor of 30,510 taken from the nightly builds ofmain.Every measurement below ran on one
ubuntu-22.04-armrunner per run, with each tree built whole in its own folder, the same TAC Enterprise data file of 109,162,108 bytes, the same 20,000 User-Agent evidence file of 2,726,867 bytes, and the order of the arms rotating each round. Figures are detections a second, as the performance example prints them.rcis slower thanmainby more than the spread of the rounds. In run 43344516953,mainat384bff2dmeasured a median of 31,842.86 (31,383.60 to 32,516.08) andrcat20868944a median of 28,302.73 (27,342.10 to 28,899.65), every round ofrcunder every round ofmain.rcwith the log masking line that #85 took off the detection path taken off them too. Every round at Give each web integration element its own switch, put Sequence first, and give it element data #53 (28,086.38 to 29,065.50) was under every round of the commit before it (29,975.51 to 31,872.31), and the three commits after it were flat.git diff --statfor each patched arm lists the named file and nothing else apart from thecxxsubmodule, which the unpatchedrcarm lists too.mainmainwith #53's engine filter alonercrcwith #53's engine filter taken outAdding the filter to
maincosts 6.1 per cent, and taking it out ofrcgains 11.7 per cent, which putsrcinsidemain's range.The change in question is where #53 made the device detection engine pass the native engine only the evidence that engine reads, in deviceDetectionOnPremise.js lines 617 to 624, and the IP Intelligence engine the same in ipIntelligenceOnPremise.js lines 472 to 477. That runs
EvidenceKeyFilter.filterEvidence, which callsBasicListEvidenceKeyFilter.filterEvidenceKeyfor each evidence key, and that function puts every key in the engine's list, and the key being checked once for each of them, in lower case on every call.Evidence.addalready makes that check for every element as each value is added, so since #53 a detection walks the list at least twice.What the specification says
evidence.md line 10:
advertize-accepted-evidence.md lines 10 to 17:
The specification asks for a list of keys compared ignoring letter case, and leaves how the comparison is made to the implementation.
What changed
basicListEvidenceKeyFilter.js keeps the list's keys in lower case in a
Set, made on the first check and made again whenever the list is replaced, grows, shrinks or has a key changed. A check then puts only the key being checked in lower case and looks it up.listis public and a caller may change it. Knowing whether it changed costs one comparison of each key's reference per check, against two calls totoLowerCasefor each key before.#private;line thattscwrites for the private fields. The green run regenerates it and finds no difference.Tests
tests/evidenceKeyFilterCost.test.js gives each key in a list its own
toLowerCasethat counts its calls.checking keys puts the list in lower case once, 200 checks against a list of 10 keys, asking for 10.a detection does not put the list in lower case again, 25 detections through a pipeline whose element reads its evidence throughfilterEvidenceas the engines do, asking for 10 and checking each detection keptheader.user-agentalone.filterEvidencekeeping the listed keys with their values.Runs
1d851e52, which isrcwith the new test file and a throwaway test workflow and no production change. 2 failed, 1,140 passed, 1,142 total. The two are tests 1 and 2 above, counting 2000 and 750 where 10 is asked.69d18342, which is this head with the same throwaway workflow. 1,142 passed, 1,142 total. ESLint reports 0 errors and 188 warnings on both runs, none of them in the two changed source files.76eb8e7a, which differs from this head only in one blank line in the test file and the place of the#private;line in the type declaration.mainrcThe draft measures 3.4 per cent above
main's median and insidemain's range.Setting 'PullRequestSha'lines read818c435437b7fbdc1309ab4195eba0859075a99b, which is this head, Configure printed "Left out 2 macOS entries", and each of the 6 jobs ran two suites, being 1,355 passed with 2 skipped and 132 passed 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
mainit measured 2.9 per cent lower with the rounds overlapping. This draft more than makes that up, as the measurement above shows, and it does not change Read and store evidence keys without regard to letter case #71.filterEvidenceKeyreturnsundefinedrather thanfalsefor a key it does not keep, as it did before. The specification says false. This draft leaves that alone, so that cost is the only thing it changes.