Closed Bug 1121973 Opened 5 years ago Closed 5 years ago

Remove the unneeded FrameState structure from the stack capture code

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
Points:
2

Tracking

()

RESOLVED FIXED
mozilla38
Iteration:
38.3 - 23 Feb
Tracking Status
firefox38 --- fixed

People

(Reporter: Paolo, Assigned: Paolo)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

Remove the unneeded FrameState structure from the stack capture code.
Attached patch The patch (obsolete) — Splinter Review
https://treeherder.mozilla.org/#/jobs?repo=try&revision=04c4d3bb6074
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Attachment #8549609 - Flags: review?(nfitzgerald)
Comment on attachment 8549609 [details] [diff] [review]
The patch

Review of attachment 8549609 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/SavedStacks.cpp
@@ +92,5 @@
>    private:
> +    SavedFrame::AutoLookupRooter *ref;
> +};
> +
> +class SavedFrame::AutoLookupVector : public JS::CustomAutoRooter {

Uh, the MOZSTACK_CLASS change didn't make it to the patch. I've updated it now.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=433165ae151b
Attached patch This actually works (obsolete) — Splinter Review
Okay, I've been a little bit too optimistic in assuming the fact the code built without errors meant it had memory correctness.

In the end I had to use two separate rooting classes, a rooting vector of Lookup* and the normal AutoLookupRooter. The memory handling is the best I came up with, but Lookup instances have to be allocated with "new" and "delete" explicitly. I'd ordinarily have used nsRefPtr but it seems it's not available here. Suggestions for cleaning this up are welcome.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb16f1c12285

In retrospect, I think the old code might have been copy constructing much more than needed.
Attachment #8549609 - Attachment is obsolete: true
Attachment #8549609 - Flags: review?(nfitzgerald)
Attachment #8549671 - Flags: review?(nfitzgerald)
Comment on attachment 8549671 [details] [diff] [review]
This actually works

Review of attachment 8549671 [details] [diff] [review]:
-----------------------------------------------------------------

Rather than using new/delete directly, you should choose the appropriate function based on the scenario: https://dxr.mozilla.org/mozilla-central/source/js/public/Utility.h?from=js_new#141-181

Additionally, ScopedDeletePtr can help clean up failure branches: https://dxr.mozilla.org/mozilla-central/source/mfbt/Scoped.h?from=ScopedDeletePtr#238-243 (Here is some example usage: https://dxr.mozilla.org/mozilla-central/source/js/src/vm/UbiNode.cpp#209)

However, I think we can avoid all of that here.

Rather than defining copy constructors for `SavedFrame::Lookup` or using pointers, let's define move constructors (`Lookup(Lookup &&rhs)`) which transfer ownership of internal data rather than copying it. This lets us avoid messing with raw pointers and incurring malloc overhead for a stack-lifetime object, but also avoids the performance degradation of unnecessary copy construction. Best of both worlds!

https://dxr.mozilla.org/mozilla-central/source/mfbt/Vector.h?from=VectorBase#510-515
https://dxr.mozilla.org/mozilla-central/source/mfbt/Move.h#16-193
Attachment #8549671 - Flags: review?(nfitzgerald)
(Sorry, I should have caught this before, when you flagged me in bug 1083359)
Attached patch Another approachSplinter Review
Thanks for all the information! It was really helpful.

I've then seen we can implement the much simpler technique of initializing Vector's memory in place, that is another step forward from the move constructor idea:

http://mxr.mozilla.org/mozilla-central/source/js/src/jit/MIR.h#6412

I've also found the RootedGeneric template class from bug 1020690 that works on any object implementing a "trace" method:

http://mxr.mozilla.org/mozilla-central/source/js/src/vm/Shape-inl.h#133

Unfortunately, it has no support for mutability (no "set" method), and apparently doesn't work with JS::Handle<T> and JS::MutableHandle<T>. Any way of making the two work together (best), or alternatively define new template classes (if the former cannot be done)?

See for example how we implement our own AutoRooter and MutableHandle classes for LocationValue, that could easily be one-liners.
Attachment #8549671 - Attachment is obsolete: true
Attachment #8550976 - Flags: review?(nfitzgerald)
Comment on attachment 8550976 [details] [diff] [review]
Another approach

Review of attachment 8550976 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

> Unfortunately, it has no support for mutability (no "set" method), and
> apparently doesn't work with JS::Handle<T> and JS::MutableHandle<T>.
> Any way of making the two work together (best), or alternatively define
> new template classes (if the former cannot be done)?

You could always add a set method, no?

As for expanding it to support handle vs mutable handle: I think you could do by defining HandleGeneric<T> and MutableHandleGeneric<T> and adding the various implicit constructors.

The patch looks good to me as-is, FWIW.
Attachment #8550976 - Flags: review?(nfitzgerald) → review+
I saw one or two braced single-line loop bodies that shouldn't be braced, FYI.

(In reply to Nick Fitzgerald [:fitzgen] from comment #4)
> Additionally, ScopedDeletePtr can help clean up failure branches:
> https://dxr.mozilla.org/mozilla-central/source/mfbt/Scoped.
> h?from=ScopedDeletePtr#238-243 (Here is some example usage:
> https://dxr.mozilla.org/mozilla-central/source/js/src/vm/UbiNode.cpp#209)

Scoped as a class is deprecated, and ScopedDeletePtr is *particularly* deprecated.  You should be using UniquePtr instead.
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #8)

Thanks for catching these!
Blocks: 1132042
Points: --- → 2
Flags: qe-verify-
Flags: firefox-backlog+
Iteration: --- → 38.3 - 23 Feb
https://hg.mozilla.org/mozilla-central/rev/f8c367a18f16
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.