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

potential speed up in json encoding #372

Closed
jku opened this issue Oct 8, 2021 · 2 comments · Fixed by #410
Closed

potential speed up in json encoding #372

jku opened this issue Oct 8, 2021 · 2 comments · Fixed by #410

Comments

@jku
Copy link
Collaborator

jku commented Oct 8, 2021

canonical json encoding turns out to be in the hot path for most TUF tests (this happens if signing or verifying signatures is in the hot path).

Trying to heavily optimize this is probably not a good goal for securesystemslib but we could see if we can improve the code and improve performance at the same time... I'm looking at especially _canonical_string_encoder() which does essentially this:

return '"%s"' % re.sub(r'(["\\])', r'\\\1', string)

The obvious speedup is to compile the regex: this makes _canonical_string_encoder() >10% faster immediately.

But I wonder if it can be replaced by this (and is it clearly faster):

return '"%s"' % string.replace('\\','\\\\').replace('"', '\\"')
@jku jku added the enhancement label Oct 8, 2021
@jku
Copy link
Collaborator Author

jku commented Oct 11, 2021

It is clearly faster: the actual _canonical_string_encoder() runtime is 75% smaller, and my TUF test case (that creates metadata and then loads it with Updater) runtime is 25% smaller.

So I think the question is, do the two calls really always have the same results?

@trishankatdatadog
Copy link
Contributor

Nice find.

So I think the question is, do the two calls really always have the same results?

Maybe best to test this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants