Skip to content

perf - do not use class for Codicon#171234

Closed
bpasero wants to merge 2 commits intomainfrom
ben/proud-snake
Closed

perf - do not use class for Codicon#171234
bpasero wants to merge 2 commits intomainfrom
ben/proud-snake

Conversation

@bpasero
Copy link
Copy Markdown
Member

@bpasero bpasero commented Jan 13, 2023 •
edited
Loading

This speeds up codicons.js module load time 100% for me:

Before
Screenshot 2023-01-13 at 09 48 57

After
Screenshot 2023-01-13 at 09 48 12

@bpasero bpasero enabled auto-merge (squash) January 13, 2023 08:49
@bpasero bpasero requested a review from aeschli January 13, 2023 08:50
@bpasero bpasero self-assigned this Jan 13, 2023
@bpasero bpasero added this to the January 2023 milestone Jan 13, 2023
bpasero
bpasero commented Jan 13, 2023
View reviewed changes
Comment thread src/vs/base/common/codicons.ts
}

export interface ICodicon extends CSSIcon {
readonly codiconFontCharacter: string;
Copy link
Copy Markdown
Member Author

@bpasero bpasero Jan 13, 2023

Choose a reason for hiding this comment

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

@aeschli fyi I decided here to name fontCharacter actually codiconFontCharacter to be able to implement a good isCodicon method that can check on that. Previously we were able to do instanceof Codicon, but with this change we no longer can. If we kept fontCharacter, there is a chance we would wrongly match on an object that has both id and fontCharacter. Since I found a few more types with fontCharacter I decided to go this path, but please verify that is OK.

@bpasero bpasero mentioned this pull request Jan 13, 2023
Merged
@bpasero
Copy link
Copy Markdown
Member Author

bpasero commented Jan 13, 2023

Hm, I wonder if we can even get rid of namespace and use something along the lines of:

export const Codicon = {

	// built-in icons, with image name
	'add': makeCodicon('add', '\\ea60'),

Because looking at microsoft/TypeScript#51387, there seems to be additional lookup cost when using namespace. The only then to keep in mind is that you cannot reference another Codicon from within that object literal as we do for some icons ('treeItemExpanded': makeCodicon('tree-item-expanded', chevronDown.codiconFontCharacter))

@bpasero
Copy link
Copy Markdown
Member Author

bpasero commented Jan 13, 2023

Better done in #171254

@bpasero bpasero closed this Jan 13, 2023
auto-merge was automatically disabled January 13, 2023 13:56

Pull request was closed

@bpasero bpasero deleted the ben/proud-snake branch January 19, 2023 09:58
@github-actions github-actions Bot locked and limited conversation to collaborators Feb 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@aeschli aeschli Awaiting requested review from aeschli

Assignees

@bpasero bpasero

Labels

None yet

Projects

None yet

Milestone

January 2023

Development

Successfully merging this pull request may close these issues.

1 participant

@bpasero