Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2555433002:
[inspector] [wasm] Add folder structure to wasm urls (Closed)
|
Created:
4 years ago by Clemens Hammacher Modified:
4 years ago CC:
devtools-reviews_chromium.org, v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[inspector] [wasm] Add folder structure to wasm urls
It turns out that showing a five-digit number of resources blocks the
UI for a few minutes, and it remains very laggy even after that.
This CL adds another component to the path of wasm scripts if the
module contains more than 300 functions. The additional component will
be the function index rounded down to the next multiple of 100.
Example URL before:
wasm://wasm/wasm-0284f1c6/wasm-0284f1c6-26337
Example URL after:
wasm://wasm/wasm-0284f1c6/26300/wasm-0284f1c6-26337
This avoids showing a five-digit number of entries in the resources view.
R=kozyatinskiy@chromium.org, titzer@chromium.org, yangguo@chromium.org
BUG=chromium:659715
Committed: https://crrev.com/2da865d8a44082e542b87972c1a888205e9a949d
Cr-Commit-Position: refs/heads/master@{#41522}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Address comment #Patch Set 3 : Rebase #
Depends on Patchset: Messages
Total messages: 29 (18 generated)
The CQ bit was checked by clemensh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== [inspector] [wasm] Add folder structure to wasm urls It turns out that showing a five-digit number of resources blocks the UI for a few minutes, and it remains very laggy even after that. This CL adds another component to the path of wasm scripts if the module contains more than 300 functions. The additional component will be the function index rounded down to the next multiple of 100. Example URL before: wasm://wasm/wasm-0284f1c6/wasm-0284f1c6-26337 Example URL after: wasm://wasm/wasm-0284f1c6/26300/wasm-0284f1c6-26337 This avoids showing a five-digit number of entries in the resources view. R=kozyatinskiy@chromium.org, titzer@chromium.org BUG=chromium:659715 ========== to ========== [inspector] [wasm] Add folder structure to wasm urls It turns out that showing a five-digit number of resources blocks the UI for a few minutes, and it remains very laggy even after that. This CL adds another component to the path of wasm scripts if the module contains more than 300 functions. The additional component will be the function index rounded down to the next multiple of 100. Example URL before: wasm://wasm/wasm-0284f1c6/wasm-0284f1c6-26337 Example URL after: wasm://wasm/wasm-0284f1c6/26300/wasm-0284f1c6-26337 This avoids showing a five-digit number of entries in the resources view. R=kozyatinskiy@chromium.org, titzer@chromium.org, yangguo@chromium.org BUG=chromium:659715 ==========
clemensh@chromium.org changed reviewers: + yangguo@chromium.org
This CL is actually fixing a frontend problem (too many elements in the tree view, or whatever this expandable thing for showing resources is called). I would actually prefer a general fix for this in the frontend, instead of tweaking the reported URLs. Do you think this is doable? As an intermediate step, we could land this CL now, and revert it later when frontend support for this is added.
https://codereview.chromium.org/2555433002/diff/1/src/inspector/wasm-translat... File src/inspector/wasm-translation.cc (right): https://codereview.chromium.org/2555433002/diff/1/src/inspector/wasm-translat... src/inspector/wasm-translation.cc:147: return 0; This seems just to left-pad the folder name to the same width as the max value. Is this that important? Or can we just always pad to 8 digits to save on complexity?
The CQ bit was checked by clemensh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2555433002/diff/1/src/inspector/wasm-translat... File src/inspector/wasm-translation.cc (right): https://codereview.chromium.org/2555433002/diff/1/src/inspector/wasm-translat... src/inspector/wasm-translation.cc:147: return 0; On 2016/12/05 at 15:16:42, Yang wrote: > This seems just to left-pad the folder name to the same width as the max value. Is this that important? Or can we just always pad to 8 digits to save on complexity? Yes, this is just for aesthetics. We can either just replace it by 8, or by String16::fromInteger(number).length(). As the complexity of the code below is not reduced by switching to "always 8", I would actually prefer the String16::fromInteger solution, which is much closer to the complexity of a constant than the current code, but still produces nice results :) I changed it accordingly.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...) v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...) v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...) v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/17341) v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...) v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/17399) v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...) v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...) v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...) v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...) v8_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/29999) v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/18917) v8_win_compile_dbg on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/2...) v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...) v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/19041)
lgtm, Just curious, could we provide more meaningful grouping?
On 2016/12/05 at 18:34:57, kozyatinskiy wrote: > lgtm, > > Just curious, could we provide more meaningful grouping? I don't know of any. Do you think there will ever be frontend support which makes this CL superfluous? Should I open a bug for this? What I don't like about the current solution is that the URLs we generate will be visible for all embedders, even though they are just there to solve a frontend problem.
On 2016/12/05 18:37:16, Clemens Hammacher wrote: > On 2016/12/05 at 18:34:57, kozyatinskiy wrote: > > lgtm, > > > > Just curious, could we provide more meaningful grouping? > > I don't know of any. > > Do you think there will ever be frontend support which makes this CL > superfluous? Should I open a bug for this? > What I don't like about the current solution is that the URLs we generate will > be visible for all embedders, even though they are just there to solve a > frontend problem. Bug sounds good to me. I agree that we need to fix frontend part here: make it faster or group entries.
On 2016/12/05 18:41:06, kozyatinskiy wrote: > On 2016/12/05 18:37:16, Clemens Hammacher wrote: > > On 2016/12/05 at 18:34:57, kozyatinskiy wrote: > > > lgtm, > > > > > > Just curious, could we provide more meaningful grouping? > > > > I don't know of any. > > > > Do you think there will ever be frontend support which makes this CL > > superfluous? Should I open a bug for this? > > What I don't like about the current solution is that the URLs we generate will > > be visible for all embedders, even though they are just there to solve a > > frontend problem. > > Bug sounds good to me. I agree that we need to fix frontend part here: make it > faster or group entries. lgtm.
> Bug sounds good to me. I agree that we need to fix frontend part here: make it faster or group entries. Done: http://crbug.com/671581
The CQ bit was checked by clemensh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by clemensh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kozyatinskiy@chromium.org, yangguo@chromium.org Link to the patchset: https://codereview.chromium.org/2555433002/#ps40001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1481032134981420,
"parent_rev": "07a0acf6971b266a98c8e2af9217a7896c4e3707", "commit_rev":
"67dd6f1f08c817a20decf3b8e5c39a44c126d503"}
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [inspector] [wasm] Add folder structure to wasm urls It turns out that showing a five-digit number of resources blocks the UI for a few minutes, and it remains very laggy even after that. This CL adds another component to the path of wasm scripts if the module contains more than 300 functions. The additional component will be the function index rounded down to the next multiple of 100. Example URL before: wasm://wasm/wasm-0284f1c6/wasm-0284f1c6-26337 Example URL after: wasm://wasm/wasm-0284f1c6/26300/wasm-0284f1c6-26337 This avoids showing a five-digit number of entries in the resources view. R=kozyatinskiy@chromium.org, titzer@chromium.org, yangguo@chromium.org BUG=chromium:659715 ========== to ========== [inspector] [wasm] Add folder structure to wasm urls It turns out that showing a five-digit number of resources blocks the UI for a few minutes, and it remains very laggy even after that. This CL adds another component to the path of wasm scripts if the module contains more than 300 functions. The additional component will be the function index rounded down to the next multiple of 100. Example URL before: wasm://wasm/wasm-0284f1c6/wasm-0284f1c6-26337 Example URL after: wasm://wasm/wasm-0284f1c6/26300/wasm-0284f1c6-26337 This avoids showing a five-digit number of entries in the resources view. R=kozyatinskiy@chromium.org, titzer@chromium.org, yangguo@chromium.org BUG=chromium:659715 Committed: https://crrev.com/2da865d8a44082e542b87972c1a888205e9a949d Cr-Commit-Position: refs/heads/master@{#41522} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/2da865d8a44082e542b87972c1a888205e9a949d Cr-Commit-Position: refs/heads/master@{#41522} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
