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

Do not regenerate api object that does not change #99

Merged
merged 1 commit into from
Aug 24, 2022

Conversation

paololim
Copy link
Contributor

@paololim paololim commented Aug 21, 2022

The Dynamic team is using siwe-parser in our codebase in production, and noticed that ParsedMessage(text) was causing unnecessary latency in our application. When we took a closer look its implementation, we noticed that the bulk of the latency was from creating a new apgApi:

const api = new apgApi(GRAMMAR);

Doing this in the constructor of ParsedMessage is unnecessary because new apgApi(GRAMMAR) would never actually change and that GRAMMAR is a static const declared earlier in the file. This means every time we instantiate the ParsedMessage class, we generate a new and unnecessary apgApi.

This PR removes the expensive API generation from ParsedMessage's constructor and make sure we only generate the API once.

siwe-parser's tests have a bunch of test cases they evaluate against to check that the parser works. Locally, I added basic time duration that looks like:

const start = new Date().getTime();
const parsedMessage = new ParsedMessage(test.message); // this line was already in parser.test.ts
const duration = new Date().getTime() - start;
console.log(`test duration[${test_name}]: ${new Date().getTime() - start}`)

Here are the results collected from running the test BEFORE I made the change (this was running on a M1 Max):

test duration[couple of optional fields]: 53
test duration[no optional field]: 35
test duration[timestamp without microseconds]: 29
test duration[domain is RFC 3986 authority with IP]: 26
test duration[domain is RFC 3986 authority with userinfo]: 25
test duration[domain is RFC 3986 authority with port]: 21
test duration[domain is localhost authority with port]: 21
test duration[domain is RFC 3986 authority with userinfo and port]: 22
test duration[no statement]: 21
test duration[domain ipv6]: 22
test duration[uri ipv6]: 22
test duration[uri ipv4]: 21
test duration[uri with port]: 21
test duration[uri ipv4 query params and fragment]: 21
test duration[chainId not 1]: 21
test duration[recovery byte starting at 0]: 21

And here are the results collected after the change was made:

test duration[couple of optional fields]: 11
test duration[no optional field]: 2
test duration[timestamp without microseconds]: 2
test duration[domain is RFC 3986 authority with IP]: 1
test duration[domain is RFC 3986 authority with userinfo]: 1
test duration[domain is RFC 3986 authority with port]: 1
test duration[domain is localhost authority with port]: 2
test duration[domain is RFC 3986 authority with userinfo and port]: 2
test duration[no statement]: 0
test duration[domain ipv6]: 1
test duration[uri ipv6]: 2
test duration[uri ipv4]: 0
test duration[uri with port]: 0
test duration[uri ipv4 query params and fragment]: 0
test duration[chainId not 1]: 0
test duration[recovery byte starting at 0]: 0

As you can see, the duration for instantiating the ParsedMessage class goes down significantly after the change - and this would translate to much lower latency in our application and the endpoint that uses this.

@paololim paololim changed the title fix: do not regenerate api object that does not change Do not regenerate api object that does not change Aug 21, 2022
@paololim
Copy link
Contributor Author

Hi there! I just wanted to follow-up on this PR. Anything we can do to help with the review? Thank you!

@w4ll3 w4ll3 requested review from sbihel and w4ll3 August 24, 2022 15:24
@w4ll3
Copy link
Member

w4ll3 commented Aug 24, 2022

Thanks for you PR, sorry for the late reply as well. We'll be looking into it.

Copy link
Member

@sbihel sbihel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you just rebase your PR before I merge please?

@paololim
Copy link
Contributor Author

paololim commented Aug 24, 2022

hi @sbihel @w4ll3 - thanks for taking a look! i have rebased the commit off of upstream/main, and should be correct now. let me know if you need anything else.

Screen Shot 2022-08-24 at 11 50 48 AM

And also made sure tests pass locally:

[/Users/paolo/Dyn/siwe:faster_parser]☕ npm run test
> siwe@2.0.5 test
> jest
 PASS  packages/siwe-parser/lib/parsers.test.ts
 PASS  packages/siwe/lib/client.test.ts (14.525 s)
....
....
....
Test Suites: 2 passed, 2 total
Tests:       209 passed, 209 total
Snapshots:   0 total
Time:        14.825 s
Ran all test suites in 2 projects.

@skgbafa skgbafa merged commit f9bb59a into spruceid:main Aug 24, 2022
@paololim paololim deleted the faster_parser branch August 24, 2022 17:57
@paololim
Copy link
Contributor Author

@skgbafa @sbihel Thanks for reviewing and merging! When can we expect the new version to be published to NPM?

@w4ll3
Copy link
Member

w4ll3 commented Aug 25, 2022

I can't give you an exact date right now, but we have a new release planned already and this changes will be included.

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.

4 participants