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

feat: enable code cache for custom protocols #40544

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
Member

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

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 a team as a code owner November 16, 2023 07:00
@electron-cation electron-cation bot added api-review/requested 🗳 new-pr 🌱 PR opened in the last 24 hours labels Nov 16, 2023
@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
Copy link
Member 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 in the last 24 hours label Nov 23, 2023
@zcbenz zcbenz requested a review from a team November 28, 2023 00:01
jkleinsc
jkleinsc reviewed Dec 4, 2023
View reviewed changes
Copy link
Contributor

@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

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
19 checks passed
@deepak1556 deepak1556 deleted the code-cache-custom-protocol branch December 6, 2023 02:22
@release-clerk 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

@anonrig anonrig anonrig left review comments

@deepak1556 deepak1556 deepak1556 approved these changes

@miniak miniak miniak approved these changes

@samuelmaddock samuelmaddock samuelmaddock approved these changes

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