Correct compiled code checks for pretenured flags

RESOLVED FIXED in Firefox 65

Status

()

enhancement
P3
normal
RESOLVED FIXED
9 months ago
9 months ago

People

(Reporter: mgaudet, Assigned: mgaudet)

Tracking

Trunk
mozilla65
Points:
---

Firefox Tracking Flags

(firefox64 wontfix, firefox65 fixed)

Details

Attachments

(2 attachments)

Comparing the implementation of ObjectGroup::shouldPreTenure() [1] with the check in the generated unboxed object constructor [2] reveals an inconsistency that leads to more object pretenuring than desired.


[1]: https://searchfox.org/mozilla-central/rev/0ec9f5972ef3e4a479720630ad7285a03776cdfc/js/src/vm/ObjectGroup-inl.h#66-70
[2]: https://searchfox.org/mozilla-central/rev/0ec9f5972ef3e4a479720630ad7285a03776cdfc/js/src/vm/UnboxedObject.cpp#138-139
OBJECT_FLAG_PRE_TENURE is contained within OBJECT_FLAG_DYNAMIC_MASK, and so it
is set not only when pretenuring is required, but also whenever
OBJECT_FLAG_UNKNOWN_PROPERTIES is set. By not checking the
OBJECT_FLAG_UNKNOWN_PROPERTIES flag, the constructor will tenure allocate any
objects with the OBJECT_FLAG_UNKNOWN_PROPERTIES bit set, which may be overly
aggressive.
A performance push for comparison: https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=95dec13e3c3d&framework=1&showOnlyComparable=1&selectedTimeRange=172800 

Most results are low-confidence (especially  if you lengthen the timescale for comparison) and so I don't think this should be too impactful, but helps ensure correctness.
Curiously, and I haven't investigated this much further, but if you patch MacroAssembler::branchIfPretenuredGroup 

>  void
>  MacroAssembler::branchIfPretenuredGroup(Register group, Label* label)
>  {
> +    // To check for the pretenured flag we need OBJECT_FLAG_PRETENURED set, and
> +    // OBJECT_FLAG_UNKNOWN_PROPERTIES unset, so check the latter first, and don't
> +    // branch if it set.
> +    Label unknownProperties;
> +    branchTest32(Assembler::NonZero, Address(group, ObjectGroup::offsetOfFlags()),
> +                Imm32(OBJECT_FLAG_UNKNOWN_PROPERTIES), &unknownProperties);
>      branchTest32(Assembler::NonZero, Address(group, ObjectGroup::offsetOfFlags()),
>                   Imm32(OBJECT_FLAG_PRE_TENURE), label);
> +    bind(&unknownProperties);
>  }

in a similar fashion, the JIT tests start to fail. I suspect there's been a bit of a semantic blurring that has occurred between the notion of OBJECT_FLAG_UNKNOWN_PROPERTIES and OBJECT_FLAG_PRE_TENURE.
(In reply to Matthew Gaudet (he/him) [:mgaudet] from comment #3)
> Curiously, and I haven't investigated this much further, but if you patch
> MacroAssembler::branchIfPretenuredGroup
> [...]
> in a similar fashion, the JIT tests start to fail.

Mind filing a follow-up bug? This is interesting.
Summary: Correctly check for pretenured flag in generated unboxed object constructor → Correctly JIT checks for pretenured flags
Summary: Correctly JIT checks for pretenured flags → Correct compiled code checks for pretenured flags
Pushed by mgaudet@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a90b3d91f1a4
Correct check for pretenured flag in unboxed objects constructors r=jandem
https://hg.mozilla.org/integration/autoland/rev/486615f18dd7
Correct branchIfPretenuredGroup r=jandem
https://hg.mozilla.org/mozilla-central/rev/a90b3d91f1a4
https://hg.mozilla.org/mozilla-central/rev/486615f18dd7
Status: NEW → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
You need to log in before you can comment on or make changes to this bug.