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

Try mangling _private service props #165117

Closed
wants to merge 3 commits into from
Closed

Conversation

mjbvz
Copy link
Collaborator

@mjbvz mjbvz commented Nov 1, 2022

This enables property mangling for properties with names such as _someService. This is designed to only touch properties that we are confident can be mangled safely (in this case, any private services)

This results in a very small (0.75%) reduction in the size of workbench.js. For the future:

  • If we mangle every this.someService property, we could reduce the bundle size by 4%

  • If we mangle every this._privateProp properties, we could reduce the bundle size by 6%. We could likely save more by adopting _ names for private props in more places

  • If we mangle every property, we'd reduce by bundle size by 40% (this is only theoretically, as doing this completely breaks the code)

Results in a slight reduction in code size. Needs more testing
@mjbvz mjbvz added this to the November 2022 milestone Nov 1, 2022
@mjbvz mjbvz self-assigned this Nov 1, 2022
@mjbvz mjbvz marked this pull request as ready for review November 1, 2022 06:36
@mjbvz mjbvz requested a review from jrieken November 1, 2022 06:39
@jrieken
Copy link
Member

jrieken commented Nov 1, 2022

Change looks good, unsure about the CI failure tho

@mjbvz mjbvz requested review from joaomoreno and alexdima November 1, 2022 18:08
@jrieken
Copy link
Member

jrieken commented Nov 2, 2022

Could this be failing because folks use _ for protected services that are now being mangled?

@mjbvz
Copy link
Collaborator Author

mjbvz commented Nov 2, 2022

@jrieken Where were seeing the failures? I created a private build (13c08c6). It failed for unrelated reasons. I will kick off another test build though

I also sanity checked the bits that succeeded and everything seems to run ok

@jrieken
Copy link
Member

jrieken commented Nov 3, 2022

Sorry, saw the wrong/bogous build failure. Anyways, I do believe there is an inherent risk with this or I am missing something. We have many injected services using _fffService that are protected properties (sample). Aren't we breaking those with the mangle?

As a related FYI: I have updated #165259 to only check for _\w+Service which is a smaller, but not small, list. This build has a list of protected properties that use the service naming pattern: https://dev.azure.com/vscode/a4cdce18-a05c-4bb8-9476-5d07e63bfd76/_build/results?buildId=77891

@mjbvz
Copy link
Collaborator Author

mjbvz commented Nov 3, 2022

@jrieken My understanding of property mangling is that it operates on the entire bundle (not per file). If a protected property is managed, all the subclasses using it should also get updated to use the new name. I will confirm this is happening correctly in our codebase

@mjbvz
Copy link
Collaborator Author

mjbvz commented Nov 15, 2022

Closing in favor of #166126

@mjbvz mjbvz closed this Nov 15, 2022
@mjbvz mjbvz deleted the dev/mjbvz/mangle-private-service branch December 5, 2022 19:52
@github-actions github-actions bot locked and limited conversation to collaborators Dec 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants