Mitigate perf degradation cause by loading wallet-standard #16868
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
darkdh
force-pushed
the
wallet-standard-perf
branch
from
d9476cd to
a173a77
Compare
darkdh
changed the title
bbondy
approved these changes
Jan 26, 2023
StephenHeaps
approved these changes
Jan 26, 2023
nuo-xu
approved these changes
Jan 26, 2023
atuchin-m
reviewed
Jan 27, 2023
| @@ -211,6 +211,9 @@ interface SolanaProvider { | |||
| => (SolanaProviderError error, string error_message, | |||
| mojo_base.mojom.DictionaryValue result); | |||
|
|
|||
| // DO NOT USE: This is supposed to be a temoprary solution for us to decide | |||
There was a problem hiding this comment.
nit:
Suggested change
| // DO NOT USE: This is supposed to be a temoprary solution for us to decide | |
| // DO NOT USE: This is supposed to be a temporary solution for us to decide |
| @@ -600,6 +600,12 @@ void SolanaProviderImpl::Request(base::Value::Dict arg, | |||
| } | |||
| } | |||
|
|
|||
| void SolanaProviderImpl::IsSolanaKeyringCreated( | |||
| IsSolanaKeyringCreatedCallback callback) { | |||
| std::move(callback).Run( | |||
There was a problem hiding this comment.
nit: add DCHECK(keyring_service_);
| @@ -68,6 +70,10 @@ JSSolanaProvider::JSSolanaProvider(content::RenderFrame* render_frame) | |||
| *g_provider_solana_web3_script = | |||
| LoadDataResource(IDR_BRAVE_WALLET_SOLANA_WEB3_JS); | |||
| } | |||
| if (g_wallet_standard_script->empty()) { | |||
| *g_wallet_standard_script = LoadDataResource( | |||
There was a problem hiding this comment.
Do we really need to cache LoadDataResource() returned values?
If we need, then it should be shared memory for all the renderer. Otherwise it takes 1.8MB(the string size) per renderer process because it's private process memory.
There was a problem hiding this comment.
For large resources:
- If the resource isn't gzipped use
GetRawDataResource()each time, don't cache; - If unzip time is affordable don't cache, use
LoadDataResourceString()each time (except large resources on UI thread in browser process); - Store cached unzipped resources in browser process. Pass them via IPC each time you need it. Or store it in shared memory and pass a handle.
Bad practice:
- Using private process memory to cache
LoadDataResourceString()(i.e. static std::string) for large resources. It increases total footprint byRESOURCE_SIZE * NwhereNis renderer process number. Also each new tab usually uses a new process => old cache doesn't help to save time. - Loading large zipped resources on UI thread. It will result in freezes/frame drops: crbug.com/973417
There was a problem hiding this comment.
In this particular case all 3 cached resources are unzipped => please use GetRawDataResource() each time, don't cache.
P.S. I wonder could we support gzip in transpile_web_ui.
There was a problem hiding this comment.
Those cached scripts resources are removed and they are only be used once, I don't think they need to be cached.
atuchin-m
approved these changes
Jan 27, 2023
darkdh
force-pushed
the
wallet-standard-perf
branch
2 times, most recently
from
2613037 to
31fd637
Compare
darkdh
force-pushed
the
wallet-standard-perf
branch
from
31fd637 to
2c655a2
Compare
darkdh
added a commit
that referenced
this pull request
Jan 28, 2023
Mitigate perf degradation cause by loading wallet-standard
7 tasks
darkdh
added a commit
that referenced
this pull request
Jan 28, 2023
7 tasks
kjozwiak
pushed a commit
that referenced
this pull request
Jan 30, 2023
This was referenced Jan 30, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves brave/brave-browser#27997
We now load wallet-standard script separately in
OnFinishLoadand only when Solana keyring is created.follow-up issue created: brave/brave-browser#28082
iOS now needs to load new script
IDR_BRAVE_WALLET_SCRIPT_WALLET_STANDARD_SCRIPT_BUNDLE_JSin order to get wallet-standard compatibility and also callIsSolanaKeyringCreatedto decide whether to load wallet-standardRelease build comparison on example.com: (Reland: v1.49.56, No wallet-standard: v1.48.110)
task (compile time + function call) is delayed after first paint
Submitter Checklist:
QA/YesorQA/No;release-notes/includeorrelease-notes/exclude;OS/...) to the associated issuenpm run test -- brave_browser_tests,npm run test -- brave_unit_testswikinpm run lint,npm run presubmitwiki,npm run gn_check,npm run tslintgit rebase master(if needed)Reviewer Checklist:
gnAfter-merge Checklist:
changes has landed on
Test Plan:
Fresh profile
--user-data-dircan be used so you can open them simultaneously)Function(anonymous) @ wallet_standard.js:1:10) + Compile time appeared after FP and FCPWith Solana account created
--user-data-dircan be used so you can open them simultaneously)Function(anonymous) @ wallet_standard.js:1:10) + Compile time ahead of it should be similar to previous version ofFunction(anonymous) @ solana_provider.js:1:10