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.

Sign up for GitHub

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

Jump to bottom

Improve GSNode.read* performance #732

Merged
merged 4 commits into from
Oct 13, 2021
Merged

Improve GSNode.read* performance #732

merged 4 commits into from
Oct 13, 2021

Conversation

madig
Copy link
Collaborator

@madig madig commented Oct 7, 2021 •
edited
Loading

Improves reading speed for v2 Glyphs.app files (as done in https://github.com/madig/noto-amalgamated) by roughly 10%:

> hyperfine -m 2 'venv_this_pr/bin/python amalgamate-noto.py' 'venv_previous/bin/python amalgamate-noto.py'
Benchmark #1: venv_this_pr/bin/python amalgamate-noto.py
  Time (mean ± σ):     265.923 s ±  4.891 s    [User: 259.716 s, System: 5.978 s]
  Range (min … max):   262.465 s … 269.381 s    2 runs

Benchmark #2: venv_previous/bin/python amalgamate-noto.py
  Time (mean ± σ):     289.823 s ±  0.376 s    [User: 285.430 s, System: 4.185 s]
  Range (min … max):   289.558 s … 290.089 s    2 runs

Summary
  'venv_this_pr/bin/python amalgamate-noto.py' ran
    1.09 ± 0.02 times faster than 'venv_previous/bin/python amalgamate-noto.py'

@schriftgestalt
some performance improvements
71a968f 
- add the nodes directly and don’t go though the proxy
- calling an empty `__init__` and then setting the properies makes two `Points()`
- use __slots__ helps quite a bit, too
@google-cla
Copy link

google-cla bot commented Oct 7, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@madig
Copy link
Collaborator Author

madig commented Oct 7, 2021

@googlebot I consent.

@google-cla
Copy link

google-cla bot commented Oct 7, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla
Copy link

google-cla bot commented Oct 7, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla
Copy link

google-cla bot commented Oct 7, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@madig
Copy link
Collaborator Author

madig commented Oct 7, 2021

@schriftgestalt can you please give your Ok to the Google bot?

@schriftgestalt
Copy link
Collaborator

@googlebot I consent.

@madig madig mentioned this pull request Oct 7, 2021
Closed
@madig
Copy link
Collaborator Author

madig commented Oct 7, 2021

@schriftgestalt thanks. Can node_type ever be None?

@schriftgestalt
Copy link
Collaborator

Don’t think so. At least not on purpose. It defaults to LINE in the initialisation.
Maybe when there is a file corruption. Or I add a new type.

and the file I tested with saw a much bigger improvement. Need to time the before and after again but it was more like 25%.

@madig
Copy link
Collaborator Author

madig commented Oct 7, 2021

Hm, what benchmark were you using?

@schriftgestalt
Copy link
Collaborator

schriftgestalt commented Oct 7, 2021 •
edited
Loading

The same as you. It probably has to do with the file you use.

@madig
Copy link
Collaborator Author

madig commented Oct 8, 2021

Hm. I'm just using a recent noto-source checkout. Anyway, any new numbers with this PR?

@madig madig merged commit 3557c9e into main Oct 13, 2021
@madig madig deleted the gs_performance branch October 13, 2021 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anthrotype anthrotype anthrotype approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@madig @schriftgestalt @anthrotype