Skip to content

feat: enable code cache for custom protocols #40544

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
Dec 6, 2023
Merged

feat: enable code cache for custom protocols #40544

merged 1 commit into from
Dec 6, 2023

Conversation

zcbenz
Copy link
Contributor

@zcbenz zcbenz commented Nov 16, 2023 •
edited by deepak1556
Loading

Description of Change

Add an option in protocol.registerSchemesAsPrivileged to allow V8 code cache in custom schemes.

Upstream Chromium issue: https://bugs.chromium.org/p/chromium/issues/detail?id=1500437.

/cc @deepak1556 and @bpasero since there will need changes in VS Code.

Fixes #27075

Checklist

Release Notes

Notes: Add an option in protocol.registerSchemesAsPrivileged to allow V8 code cache in custom schemes.

@zcbenz zcbenz added the semver/minor backwards-compatible functionality label Nov 16, 2023
@zcbenz zcbenz requested a review from deepak1556 November 16, 2023 07:00
@zcbenz zcbenz requested a review from a team as a code owner November 16, 2023 07:00
@zcbenz zcbenz added target/27-x-y PR should also be added to the "27-x-y" branch. target/28-x-y PR should also be added to the "28-x-y" branch. labels Nov 16, 2023
@deepak1556
Copy link
Member

Should we enforce a good default be only allowing code cache for standard custom schemes, since the cache key relies on origin lock of the process when available. Custom protocols with null origin will have their code cache grouped together and I am not sure cache will be useful in that case since there is more chance of collision.

@zcbenz zcbenz force-pushed the code-cache-custom-protocol branch from b556fc1 to 976689f Compare November 21, 2023 01:36
@zcbenz
Copy link
Contributor Author

zcbenz commented Nov 21, 2023

Should we enforce a good default be only allowing code cache for standard custom schemes, since the cache key relies on origin lock of the process when available.

I updated the API to throw when trying to enabling code cache for non-standard scheme.

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened recently label Nov 23, 2023
@zcbenz zcbenz requested a review from a team November 28, 2023 00:01
anonrig
anonrig reviewed Nov 30, 2023
View reviewed changes
@zcbenz zcbenz force-pushed the code-cache-custom-protocol branch from 132f235 to 2aad9dd Compare November 30, 2023 23:56
jkleinsc
jkleinsc reviewed Dec 4, 2023
View reviewed changes
Copy link
Member

@jkleinsc jkleinsc left a comment

Choose a reason for hiding this comment

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

API LGTM

@zcbenz zcbenz force-pushed the code-cache-custom-protocol branch from 1453c59 to 035726d Compare December 5, 2023 01:20
samuelmaddock
samuelmaddock approved these changes Dec 6, 2023
View reviewed changes
Copy link
Member

@samuelmaddock samuelmaddock left a comment

Choose a reason for hiding this comment

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

API LGTM

@deepak1556 deepak1556 merged commit 9aa73ab into main Dec 6, 2023
@deepak1556 deepak1556 deleted the code-cache-custom-protocol branch December 6, 2023 02:22
@release-clerk
Copy link

release-clerk bot commented Dec 6, 2023

Release Notes Persisted

Add an option in protocol.registerSchemesAsPrivileged to allow V8 code cache in custom schemes.

This was referenced Dec 6, 2023
Merged
Merged
@trop
Copy link
Contributor

trop bot commented Dec 6, 2023

I have automatically backported this PR to "27-x-y", please check out #40708

@trop trop bot added in-flight/27-x-y and removed target/27-x-y PR should also be added to the "27-x-y" branch. labels Dec 6, 2023
@trop
Copy link
Contributor

trop bot commented Dec 6, 2023

I have automatically backported this PR to "28-x-y", please check out #40709

@trop trop bot added in-flight/28-x-y and removed target/28-x-y PR should also be added to the "28-x-y" branch. labels Dec 6, 2023
MrHuangJser pushed a commit to MrHuangJser/electron that referenced this pull request Dec 11, 2023
@zcbenz
feat: enable code cache for custom protocols (electron#40544)
8588da9
@trop trop bot added merged/27-x-y PR was merged to the "27-x-y" branch. merged/28-x-y PR was merged to the "28-x-y" branch. and removed in-flight/27-x-y in-flight/28-x-y labels Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jkleinsc jkleinsc jkleinsc left review comments

@deepak1556 deepak1556 deepak1556 approved these changes

@miniak miniak miniak approved these changes

@samuelmaddock samuelmaddock samuelmaddock approved these changes

+1 more reviewer

@anonrig anonrig anonrig left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
api-review/approved ✅ merged/27-x-y PR was merged to the "27-x-y" branch. merged/28-x-y PR was merged to the "28-x-y" branch. semver/minor backwards-compatible functionality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Code cache doesn't persist for custom standard protocols
6 participants
@zcbenz @deepak1556 @jkleinsc @miniak @samuelmaddock @anonrig