If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Call propagateOOM in loadConstantDouble etc

RESOLVED FIXED in Firefox 45

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jandem, Assigned: bbouvier)

Tracking

unspecified
mozilla45
Points:
---

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(4 attachments)

(Reporter)

Description

2 years ago
In loadConstantDouble and similar methods, we do:

  dbl->uses.append(CodeOffsetLabel(j.offset()));

We should probably call propagateOOM() where we do this.

Noticed this when reviewing bug 1229057, I think this changed with bug 1181612.
Flags: needinfo?(benj)
(Assignee)

Comment 1

2 years ago
Created attachment 8694277 [details] [diff] [review]
propagateoom.patch

Indeed. Plus there are a few refactorings we can do here...
Assignee: nobody → benj
Status: NEW → ASSIGNED
Flags: needinfo?(benj)
Attachment #8694277 - Flags: review?(jdemooij)
(Assignee)

Comment 2

2 years ago
Created attachment 8694316 [details] [diff] [review]
2.template.getconstant.patch

This unifies getFloat/getDouble/getSimd into a single template function. We have to use two template parameters: one is the Constant type, the other is the Map type (as for Float/Double the hashing policy is the default one, but not for the SimdData one).
Attachment #8694316 - Flags: review?(jdemooij)
(Assignee)

Comment 3

2 years ago
Created attachment 8694317 [details] [diff] [review]
3.template.constant.patch

Not sure that template aliasing is working (the comment in public/Vector.h suggests it's not, but it mentions MSVC2013 as the starting point from which it could work, and I think that's the minimal version we're using at least, so we should be good). I have got a try push running for this...
Attachment #8694317 - Flags: review?(jdemooij)
(Assignee)

Comment 4

2 years ago
Created attachment 8694318 [details] [diff] [review]
4.template.constant.merging.patch

Merging constants in the asm.js merging scheme can easily be done with a single function and templates, without depending on patch 3. This eliminates a few redundancies.
Attachment #8694318 - Flags: review?(jdemooij)
(Assignee)

Comment 5

2 years ago
Fwiw, the try server build returned all green, even with patch 3. 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=119995703da8
(Reporter)

Comment 6

2 years ago
Comment on attachment 8694277 [details] [diff] [review]
propagateoom.patch

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

Thanks!
Attachment #8694277 - Flags: review?(jdemooij) → review+
(Reporter)

Comment 7

2 years ago
Comment on attachment 8694316 [details] [diff] [review]
2.template.getconstant.patch

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

Nice refactoring.

::: js/src/jit/x86-shared/MacroAssembler-x86-shared.cpp
@@ +258,5 @@
> +    return &vec[index];
> +}
> +
> +MacroAssemblerX86Shared::Float*
> +MacroAssemblerX86Shared::getFloat(float f) {

Nit: { on next line, also twice below
Attachment #8694316 - Flags: review?(jdemooij) → review+
(Reporter)

Updated

2 years ago
Attachment #8694317 - Flags: review?(jdemooij) → review+
(Reporter)

Comment 8

2 years ago
Comment on attachment 8694318 [details] [diff] [review]
4.template.constant.merging.patch

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

Great deduplication.
Attachment #8694318 - Flags: review?(jdemooij) → review+

Comment 9

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b964e554709
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7a7fcf24c49
https://hg.mozilla.org/integration/mozilla-inbound/rev/54c0681d35b0
https://hg.mozilla.org/integration/mozilla-inbound/rev/904fe1ab68b9

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1b964e554709
https://hg.mozilla.org/mozilla-central/rev/b7a7fcf24c49
https://hg.mozilla.org/mozilla-central/rev/54c0681d35b0
https://hg.mozilla.org/mozilla-central/rev/904fe1ab68b9
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.