InlineGetProp not being inlined on Mac x86-64

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Justin Lebar (not reading bugmail), Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed-in-tracemonkey])

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
It appears that InlineGetProp in js/src/methodjit/StubCalls.cpp is not being inlined on Mac x64 builds.  When I profile part of Dromaeo DOM using Instruments (the XCode 4 equivalent of Shark), InlineGetProp, called by js::mjit::stubs::GetProp, shows up in the profile.

I presume from the name and its usage that we want this function to be inlined?  Should be easy enough to force, at least on gcc.
(In reply to comment #0)
> I presume from the name and its usage that we want this function to be
> inlined?  Should be easy enough to force, at least on gcc.

In general it's hard to predict whether inlining a big function will matter. But it's worth a try to see if it helps.
(Reporter)

Comment 2

6 years ago
It seems to help a bit on the Dromaeo DOM microbenchmark element.property, which does:

  var ret;
  var elem = document.getElementById('some-element');
  time this operation in a loop:
    ret = elem.id;

Currently scores: 469 467 473 480 472 471
With inlining:    493 497 491 492 490 336

(Assuming the 336 is an outlier...)
(Reporter)

Comment 3

6 years ago
Created attachment 536374 [details] [diff] [review]
Patch v1
Attachment #536374 - Flags: review?(dmandelin)
(Reporter)

Comment 4

6 years ago
The units on the numbers from comment 2 are whatever Dromaeo uses -- I think it's runs/s.
Attachment #536374 - Flags: review?(dmandelin) → review+
(Reporter)

Comment 5

6 years ago
I'm not familiar with the process for landing JS changes.  Do I need to land on TM, or can I land directly on MC?
(In reply to comment #5)
> I'm not familiar with the process for landing JS changes.  Do I need to land
> on TM, or can I land directly on MC?

Either is acceptable, but TM is generally preferred for JS changes.
(Reporter)

Comment 7

6 years ago
http://hg.mozilla.org/tracemonkey/rev/a0f2364ea669
Whiteboard: [fixed-in-tracemonkey]
(Reporter)

Comment 8

6 years ago
This was pushed in some TM merge; closing.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.