1500920 - Correct compiled code checks for pretenured flags

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement, P3)

Description

[Matthew Gaudet (he/him) [:mgaudet]]

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

Comment 1

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Bug 1500920 - Correct check for pretenured flag in unboxed objects constructors r?jandem

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.

Comment 2

[Matthew Gaudet (he/him) [:mgaudet]]

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.

Comment 3

[Matthew Gaudet (he/him) [:mgaudet]]

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.

Comment 4

[Jan de Mooij [:jandem]]

(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.

Comment 5

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Bug 1500920 - Correct branchIfPretenuredGroup r?jandem

Depends on D9388

Comment 6

[Pulsebot]

Pushed by [e-mail]:
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

Comment 7

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/a90b3d91f1a4
https://hg.mozilla.org/mozilla-central/rev/486615f18dd7