IonMonkey: remove the instruction counters from the script counters.

RESOLVED FIXED in Firefox 31, Firefox OS v2.0

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: dougc, Assigned: dougc)

Tracking

unspecified
mozilla33
ARM
All
Points:
---

Firefox Tracking Flags

(firefox31 fixed, firefox32 fixed, firefox33 fixed, b2g-v2.0 fixed, b2g-v2.1 fixed)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
The script counter currently counts instruction bytes and spill bytes, and it does so using the size() method to obtain the current buffer position.

The size() method is only accurate on the ARM when pending pools are flushed. Using the size() method with a pending pool to flush is most likely a programming error so I would like to assert that the pools are flushed in the size() method.

It would be possible to use the buffer offset, excluding the pools, but this may not be available in future with an elastic assembler buffer.

This issue has been discussed before and the feedback received at the time was that this code can just be removed so I would like to do so and uplift this to beta (31) in support of the asm buffer work.
(Assignee)

Comment 1

4 years ago
Created attachment 8441875 [details] [diff] [review]
Remove the instruction counters from the script counters.

https://tbpl.mozilla.org/?tree=Try&rev=b614e5829cf4
Attachment #8441875 - Flags: review?(bhackett1024)
Attachment #8441875 - Flags: review?(bhackett1024) → review+
(Assignee)

Updated

4 years ago
Blocks: 1026919
(Assignee)

Comment 2

4 years ago
The try run had some failures on Windows debug build tests but I see the same failures on other close runs and I do not think they are related to this bug.
Keywords: checkin-needed
(Assignee)

Comment 3

4 years ago
Comment on attachment 8441875 [details] [diff] [review]
Remove the instruction counters from the script counters.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Feature was added in bug 814489.

User impact if declined: None, but this code is not accurate for the ARM backend and if this were not just used to generate some statistics then it would likely indicate a programming error so an assertion to catch such usage is being added in pending patches. Would like to get this uplifted to ESR 31 to avoid blocking other patches.

Testing completed (on m-c, etc.): Locally, and a try run.

Risk to taking this patch (and alternatives if risky): low.

String or IDL/UUID changes made by this patch: n/a
Attachment #8441875 - Flags: approval-mozilla-beta?
Attachment #8441875 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/integration/mozilla-inbound/rev/d504b084db82
Keywords: checkin-needed

Comment 5

4 years ago
https://hg.mozilla.org/mozilla-central/rev/d504b084db82
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Attachment #8441875 - Flags: approval-mozilla-beta?
Attachment #8441875 - Flags: approval-mozilla-beta+
Attachment #8441875 - Flags: approval-mozilla-aurora?
Attachment #8441875 - Flags: approval-mozilla-aurora+
status-firefox31: --- → affected
status-firefox32: --- → affected
status-firefox33: --- → fixed
https://hg.mozilla.org/releases/mozilla-aurora/rev/1d30ac49f168
https://hg.mozilla.org/releases/mozilla-beta/rev/bce3c9c7d8f7
status-b2g-v2.0: --- → fixed
status-b2g-v2.1: --- → fixed
status-firefox31: affected → fixed
status-firefox32: affected → fixed
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.