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

Improve "is (not) one of (sensitive)" performance by only hashing value once #80

Merged
merged 1 commit into from
Mar 14, 2023

Conversation

jdgarcia
Copy link
Contributor

Describe the purpose of your pull request

Last week my company added several thousand user IDs to a is one of (hashed) rule (since it seemed simpler than doing a mass edit to our db and using a custom attribute) and the CPU on our API server spiked. Looking through this SDK's source code I discovered there's optimizations that can be made to this rule type.

Related issues (only if applicable)

N/A

Requirement checklist (only if applicable)

  • I have covered the applied changes with automated tests.
    Not sure how I'd add a new automated test that tests timing that would be consistent among different devices, but I added a screenshot of the speed improvement on my own machine.
  • I have executed the full automated test set against my changes.
  • I have validated my changes against all supported platform versions.
    Only tested on macOS but since I just moved a line up, platform should be irrelevant.
  • I have read and accepted the contribution agreement.

Test performed against this rule set. There are 1000 IDs per line, for a total of 4000 IDs, under a "worst-case-scenario" where the target user ID does not exist in the list so it has to check every value.
image

Test Code:

const user = { identifier: "foo" };

const configCatKernel: FakeConfigCatKernel = { configFetcher: new FakeConfigFetcherWithThousandsOfIDs(), sdkType: "common", sdkVersion: "1.0.0" };
const client: IConfigCatClient = configcatClient.createClientWithAutoPoll("APIKEY", configCatKernel, { logger: configcatClient.createConsoleLogger(configcatClient.LogLevel.Error) });

let start = new Date();
console.log(await client.getValueAsync("testFeature", false, user));
console.log(`Time to evaluate one user: ${new Date().getTime() - start.getTime()}ms`);

start = new Date();
const valuesPromises = [];
for (let i = 0; i < 100; i++) {
  valuesPromises.push(client.getValueAsync("testFeature", false, user));
}

await Promise.all(valuesPromises);
console.log(`Time to evaluate 100 concurrent users: ${new Date().getTime() - start.getTime()}ms`);

Before (top) / After (bottom)
image

@jdgarcia jdgarcia requested a review from a team as a code owner March 14, 2023 03:13
@sonarcloud
Copy link

sonarcloud bot commented Mar 14, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@jdgarcia
Copy link
Contributor Author

jdgarcia commented Mar 14, 2023

There's also a way to get near-constant time but it involves editing the actual ConfigJSON (c = 'abc,def,ghi' => c = { abc: true, def: true, ghi: true }) after fetch, and I'm not sure if that's acceptable or if it could cause unforeseen issues, but if's something that's wanted I can update the PR with that change too.

image

image

image

@endret
Copy link
Contributor

endret commented Mar 14, 2023

Hi @jdgarcia,

Thank you for the changes. Your improvement in the evaluation logic is awesome and we would like to say a huge thanks.

What you mentioned as other optimization we do not want to allow because currently, we are working on a project that will resolve it in the future.

Best regards,
Endre

@endret endret merged commit a021399 into configcat:master Mar 14, 2023
@jdgarcia jdgarcia deleted the is-one-of-performance branch March 14, 2023 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants