1281061 - Stop doing string compares in SavedFrame::isSelfHosted to determine whether we're selfhosted

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

We have an atom, so should probably just do an atom compare.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Do an atom equality compare instead of a string compare on the script filename string in SavedFrame::isSelfHosted

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8763694 [details] [diff] [review]
Do an atom equality compare instead of a string compare on the script filename string in SavedFrame::isSelfHosted

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

Thanks!

Since the JS::ubi::StackFrame interface changed, you will need to update the other implementation as well: https://dxr.mozilla.org/mozilla-central/rev/2ab57664bf7cafdd64e136e341527c275fc8c3aa/devtools/shared/heapsnapshot/DeserializedNode.h#299

::: js/src/vm/CommonPropertyNames.h
@@ +322,5 @@
>      /* Function names for properties named by symbols. */ \
>      macro(Symbol_iterator_fun, Symbol_iterator_fun, "[Symbol.iterator]") \
> +    /* Not really a property name, but we use this to compare to atomized
> +       script source strings */ \
> +    macro(self_hosted, self_hosted, "self-hosted") \

s/self_hosted/selfHosted/

We turn "foo-bar" into "fooBar" for other property names[0], so we should be consistent here.

[0] https://dxr.mozilla.org/mozilla-central/rev/2ab57664bf7cafdd64e136e341527c275fc8c3aa/js/src/vm/CommonPropertyNames.h#120

Comment 3

[Boris Zbarsky [:bzbarsky]]

> you will need to update the other implementation as well

Huh.  Could have sworn I'd done a full-tree compile with this, but clearly not!

> s/self_hosted/selfHosted/

Done.

Comment 4

[Boris Zbarsky [:bzbarsky]]

But ok, now I see that HeapSnapshot.cpp has no cx in sight and wants to call isSelfHosted().. is that correct?  :(

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

`frame.isSelfHosted()`
  `-> is called by `getProtobufStackFrame`
    `-> is called by `StreamWriter::writeNode` as part of the `CoreDumpWriter` interface
      `-> is called by `HeapSnapshotTraversalHandler::operator()`
        `-> `HeapSnapshotTraversalHandler` is initialized here[0] and has access to a cx at that time

So, that's a lot of threading cx through functions and classes to add, but is mostly furious typing without thinking.

[0] https://dxr.mozilla.org/mozilla-central/rev/23dc78b7b57e9f91798ea44c242a04e112c37db0/devtools/shared/heapsnapshot/HeapSnapshot.cpp#1395

One potential complication I thought of that turns out to be ok: we do NOT need to recanonicalize a deserialized heap snapshot's stack frame's source to the "self-hosted" atom if the stack frame's source is "self-hosted" because we actually serialize and deserialize the isSelfHosted data separately and we use that for JS::ubi::Concrete<DeserializedStackFrame>::isSelfHosted[1].

[1] https://dxr.mozilla.org/mozilla-central/rev/23dc78b7b57e9f91798ea44c242a04e112c37db0/devtools/shared/heapsnapshot/DeserializedNode.h#299

Comment 6

[Boris Zbarsky [:bzbarsky]]

Oh, I'm an idiot.  StreamWriter has a JSContext* _member_ that I can use.  I'll just do that.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e8cf178dae8
Do an atom equality compare instead of a string compare on the script filename string in SavedFrame::isSelfHosted.  r=fitzgen

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/6e8cf178dae8