Stream the property name of getprop and setprop optimization sites

RESOLVED FIXED in Firefox 40

Status

()

Core
Gecko Profiler
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: shu, Unassigned)

Tracking

(Blocks: 1 bug)

unspecified
mozilla40
x86_64
Linux
Points:
---

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
Simple quality of life improvement.
(Reporter)

Comment 1

3 years ago
Created attachment 8588854 [details] [diff] [review]
Stream the property name of getprop and setprop optimization sites.
Attachment #8588854 - Flags: review?(kvijayan)
Comment on attachment 8588854 [details] [diff] [review]
Stream the property name of getprop and setprop optimization sites.

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

::: js/src/jsfriendapi.cpp
@@ +1257,5 @@
> +JS_FRIEND_API(JSAtom*)
> +js::GetPropertyNameFromPC(JSScript* script, jsbytecode* pc)
> +{
> +    if (!IsGetPropPC(pc) && !IsSetPropPC(pc))
> +        return nullptr;

Should this be an ASSERT instead?  Ensuring that there _is_ an op with a propname at |pc|?
Attachment #8588854 - Flags: review?(kvijayan) → review+
(Reporter)

Comment 3

3 years ago
(In reply to Kannan Vijayan [:djvj] from comment #2)
> Comment on attachment 8588854 [details] [diff] [review]
> Stream the property name of getprop and setprop optimization sites.
> 
> Review of attachment 8588854 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jsfriendapi.cpp
> @@ +1257,5 @@
> > +JS_FRIEND_API(JSAtom*)
> > +js::GetPropertyNameFromPC(JSScript* script, jsbytecode* pc)
> > +{
> > +    if (!IsGetPropPC(pc) && !IsSetPropPC(pc))
> > +        return nullptr;
> 
> Should this be an ASSERT instead?  Ensuring that there _is_ an op with a
> propname at |pc|?

No, since the pc given by ForEachTrackedOptimizationAttempt could be a GET/SETELEM or a CALL or something else. We shouldn't expose friendapi functions that test for the opcode of the *pc, so that's why that function tests for IsGetPropPC || IsSetPropPC.
(Reporter)

Comment 4

3 years ago
Created attachment 8589424 [details] [diff] [review]
Display the property name in the JIT Optimizations side pane.
Attachment #8589424 - Flags: review?(jsantell)
(Reporter)

Comment 5

3 years ago
Created attachment 8589429 [details] [diff] [review]
Display the property name in the JIT Optimizations side pane.
Attachment #8589424 - Attachment is obsolete: true
Attachment #8589424 - Flags: review?(jsantell)
Attachment #8589429 - Flags: review?(jsantell)
Comment on attachment 8589429 [details] [diff] [review]
Display the property name in the JIT Optimizations side pane.

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

The ellipses masking should be handled via CSS, but I was having a lot of trouble myself getting the whole pane rendered correctly, and working with many different length strings. Seeing how this panel will probably not stay the way it is for very long, this is fine IMO.
Attachment #8589429 - Flags: review?(jsantell) → review+
https://hg.mozilla.org/mozilla-central/rev/4fa628562f6f
https://hg.mozilla.org/mozilla-central/rev/9fc22f95a34b
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.