Closed Bug 1839839 Opened 10 months ago Closed 9 months ago

Avoid cloning the frames objects unless it is strictly necessary

Categories

(DevTools :: Debugger, enhancement)

enhancement

Tracking

(firefox117 fixed)

RESOLVED FIXED
117 Branch
Tracking Status
firefox117 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

Details

Attachments

(3 files)

Bug 1835985 removed some unecessary cloning of the frame objects.
But I missed that there were at least 3 other cases of such cloning.

One related to mapping frames to original locations:
https://searchfox.org/mozilla-central/rev/3424c000a7ff304b2d1efb8561a924232f7f12fc/devtools/client/debugger/src/actions/pause/mapFrames.js#36-46
Here we could avoid cloning the frame object if the location isn't mapped to an original source.

One related to WASM sources (there is nothing making it clear this code relates to WASM!):
https://searchfox.org/mozilla-central/rev/3424c000a7ff304b2d1efb8561a924232f7f12fc/devtools/client/debugger/src/actions/pause/mapFrames.js#94-118

And one last related to the computation of originalDisplayName, which could probably be computed while mapping the frames:
https://searchfox.org/mozilla-central/rev/3424c000a7ff304b2d1efb8561a924232f7f12fc/devtools/client/debugger/src/actions/pause/mapDisplayNames.js#26-27

This isn't mentioned anywhere, but "expandFrames" is specific to WASM.
I've added some comments and renames, while highlighting the difference between the two possible types of frame objects.
At the end, frame objects for non-WASM orignal sources aren't special, they only have distinct location vs generatedLocation attributes.

Only try to compute frame's originalDisplayName for frame relating to a non-WASM original source.
For WASM frames, it is already computed.
For generated frames, it isn't useful as frame.displayName is correct and provided by the server.
Also acknowledge why we are only computing original display from the select action.

This computation was only done from selectLocation action,
after the completion of the SET_SYMBOLS action. That, only if the current thread was paused.

Doing this right from the reducer helps avoiding thread's frames array duplication
unless this is strictly necessary.
It is probably also a clearer way to say this work should be done only after symbols are fetched.

Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c6978d4ba749
[devtools] Clarify expandFrames code being specific to WASM sources. r=bomsy,devtools-reviewers
https://hg.mozilla.org/integration/autoland/rev/c100c0694778
[devtools] Clarify the computation of frame's originalDisplayName. r=devtools-reviewers,bomsy
https://hg.mozilla.org/integration/autoland/rev/5a81ad4ed7c9
[devtools] Move the computation of frame's originalDisplayFrame to the reducer. r=devtools-reviewers,bomsy
Status: NEW → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → 117 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: