Skip to content

perf - simplify codicon type #170861

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
Merged
merged 1 commit into from
Jan 10, 2023
Merged

perf - simplify codicon type #170861

merged 1 commit into from
Jan 10, 2023
+538 −548

Conversation

@bpasero
Copy link
Member

@bpasero bpasero commented Jan 9, 2023 •
edited
Loading

Noticed a GC kicking in for codicons given the sheer amount of instances created on startup:

Screenshot 2023-01-09 at 15 19 40

Using simple string removes the GC for me:

Screenshot 2023-01-09 at 15 13 11

Besides, I find the code in codiconStyles.ts very questionable where we have an instance of check?

@bpasero bpasero enabled auto-merge (squash) January 9, 2023 14:16
@bpasero bpasero requested a review from aeschli January 9, 2023 14:17
@vscodenpa vscodenpa added this to the January 2023 milestone Jan 9, 2023
@bpasero
Copy link
Member Author

bpasero commented Jan 10, 2023

If you have an idea how we could make codicons.ts not instantiate Codicon unless really needed, I am open. I noticed that even electron-main drags this in even though no-body is using the type, just via imports.

@joaomoreno joaomoreno closed this Jan 10, 2023
auto-merge was automatically disabled January 10, 2023 15:04

Pull request was closed

@joaomoreno joaomoreno reopened this Jan 10, 2023
@aeschli
Copy link
Contributor

aeschli commented Jan 10, 2023

Thanks @bpasero !

Yes, the Codicon class is not really needed. I will follow up with a PR.

@bpasero bpasero enabled auto-merge (squash) January 10, 2023 15:16
@bpasero bpasero merged commit 52d6e52 into main Jan 10, 2023
@bpasero bpasero deleted the ben/selective-kangaroo branch January 10, 2023 15:21
@bpasero
Copy link
Member Author

bpasero commented Jan 13, 2023

@aeschli did that in #171234

@aeschli aeschli mentioned this pull request Jan 13, 2023
Merged
@github-actions github-actions bot locked and limited conversation to collaborators Feb 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@aeschli aeschli aeschli approved these changes

Assignees

@bpasero bpasero

Labels

perf-startup

Projects

None yet

Milestone

January 2023

Development

Successfully merging this pull request may close these issues.

5 participants

@bpasero @aeschli @joaomoreno @vscodenpa