Closed Bug 1451021 Opened 6 years ago Closed 6 years ago

Convert PropertyIteratorActor to protocol.js

Categories

(DevTools :: General, enhancement)

enhancement
Not set
normal

Tracking

(firefox61 fixed)

RESOLVED FIXED
Firefox 61
Tracking Status
firefox61 --- fixed

People

(Reporter: yulia, Assigned: nchevobbe)

References

Details

Attachments

(1 file)

As part of refactoring ObjectActor to protocol.js, we need to update PropertyIteratorActor. it is instantiated via: 
-> ObjectActor.enumProperties/enumEntries
Assignee: nobody → nchevobbe
Status: NEW → ASSIGNED
I have some test failures I need to check (one unrelated failure in inspector, another one in old debugger), but I pushed to DAMP to see the impact: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=80c7d4f34a94c15aaa467f6ec0896f29734344c5&newProject=try&newRevision=16e4dc085e1dd2082a149efee0f2b774414ea438&framework=1

objectexpand seems to be around ~1% slower.
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #1)
> I have some test failures I need to check (one unrelated failure in
> inspector, another one in old debugger), but I pushed to DAMP to see the
> impact:
> https://treeherder.mozilla.org/perf.html#/
> compare?originalProject=try&originalRevision=80c7d4f34a94c15aaa467f6ec0896f29
> 734344c5&newProject=try&newRevision=16e4dc085e1dd2082a149efee0f2b774414ea438&
> framework=1
> 
> objectexpand seems to be around ~1% slower.

FWIW I don't think a 1% regression on that single subtest should prevent landing something here. If it's due to some inherent slowness to protocol.js then fixing that (in separate bugs) should be a win across the board.
Comment on attachment 8968603 [details]
Bug 1451021 - Migrate PropertyIteratorActor to protocol.js; .

https://reviewboard.mozilla.org/r/237296/#review243268

Thanks Nicolas, it looks good.

It sounds fine to proceed with such a small regression, especially given that I still have various pending patches that may revert it.
But note that if you have another refactoring introducing significant regression,
it is the best time to look into it as looking at perf-html profiles of the regressing DAMP subtest often highlight the culprit within protocol.js
Attachment #8968603 - Flags: review?(poirot.alex) → review+
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/97a6ec0981d0
Migrate PropertyIteratorActor to protocol.js; r=ochameau.
https://hg.mozilla.org/mozilla-central/rev/97a6ec0981d0
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.