Closed Bug 1677284 Opened 4 years ago Closed 3 years ago

mozilla::Result: Avoid (implicit) fallback to PackingStrategy::Variant

Categories

(Core :: MFBT, task)

Product:
Core
Component:
MFBT
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
85 Branch
Tracking Flags:
Tracking Status
firefox85 --- fixed

People

(Reporter: sg, Assigned: sg)

References

Details

Attachments

(7 files)

Bug 1677284 - Avoid using PackingStrategy::Variant fallback for MimeResultType. r=alwu
47 bytes, text/x-phabricator-request
Details | Review
Bug 1677284 - Use std::aligned_storage_t instead of mozilla::AlignedStorage2 in PackingStrategy::NullIsOk implementation. r=emilio
47 bytes, text/x-phabricator-request
Details | Review
Bug 1677284 - Avoid using PackingStrategy::Variant fallback for AbortReasonOr. r=jandem
47 bytes, text/x-phabricator-request
Details | Review
Bug 1677284 - Avoid using PackingStrategy::Variant fallback for CPUUsageWatcherError. r=#xpcom-reviewers
47 bytes, text/x-phabricator-request
Details | Review
Bug 1677284 - Move PackingStrategy::Variant implementation to separate header file. r=emilio
47 bytes, text/x-phabricator-request
Details | Review
Bug 1677284 - Remove unused includes of Variant.h. r=andi
47 bytes, text/x-phabricator-request
Details | Review
Bug 1677284 - Fix Windows AARch64 bustage.
47 bytes, text/x-phabricator-request
Details | Review

The mozilla::Result fallback PackingStrategy::Variant is undesirable for various reasons:

  • Impact on build time
    • It pulls in the dependency to mozilla/Variant.h
    • It requires instanting mozilla::Variant which is complex
  • Impact at run-time
    • The compiler cannot easily optimize mozilla::Variant away

Most uses of mozilla::Result do not require the fallback strategy. To avoid implicitly falling back to it without noticing, the implementation of PackingStrategy::Variant is moved to a separate header, which should only be included where actually needed.

Some situations where the fallback was actually used, but this can easily be avoided, are also modified as part of this bug.

I tried to analyze the effects of avoiding the use of mozilla::Variant for Result in js::jit, but I'm having a bit of hard time interpreting that. I thought there might be effects on inlining decisions, and so I had a look at the code generated for js::jit::IonBuilder::inlineNativeCall on a LTO/release build, and looked at the function calls emitted for that function [1]. Here is the diff:

--- inlineNativeCall.before.calls       2020-11-14 14:29:35.601431677 +0100
+++ inlineNativeCall.after.calls        2020-11-14 14:29:35.576431658 +0100
@@ -1,5 +1,5 @@
       7  <abort@plt>
-      4  <JS::AddPersistentRoot(JS::RootingContext*, JS::RootKind, JS::PersistentRooted<void*>*)>
+      3  <JS::AddPersistentRoot(JS::RootingContext*, JS::RootKind, JS::PersistentRooted<void*>*)>
       4  <js::AutoEnterOOMUnsafeRegion::crash(char const*)>
       8  <js::gc::UnmarkGrayGCThingRecursively(js::gc::TenuredCell*)>
       2  <js::HeapTypeSetKey::freeze(js::CompilerConstraintList*)>
@@ -16,17 +16,17 @@
       1  <js::jit::IonBuilder::addMaybeCopyElementsForWrite(js::jit::MDefinition*, bool)>
       1  <js::jit::IonBuilder::addTypedArrayByteOffset(js::jit::MDefinition*)>
       5  <js::jit::IonBuilder::addTypedArrayLengthAndData(js::jit::MDefinition*, js::jit::IonBuilder::BoundsChecking, js::jit::MDefinition**, js::jit::MInstruction**, js::jit::MInstruction**)>
-     66  <js::jit::IonBuilder::bytecodeTypes(unsigned char*)>
+     64  <js::jit::IonBuilder::bytecodeTypes(unsigned char*)>
       1  <js::jit::IonBuilder::checkNurseryObject(JSObject*)>
       7  <js::jit::IonBuilder::constant(JS::Value const&)>
       1  <js::jit::IonBuilder::convertToBoolean(js::jit::MDefinition*)>
       1  <js::jit::IonBuilder::ensureArrayIteratorPrototypeNextNotModified()>
       1  <js::jit::IonBuilder::initArrayElementFastPath(js::jit::MNewArray*, js::jit::MDefinition*, js::jit::MDefinition*, bool)>
       1  <js::jit::IonBuilder::inlineArrayPopShift(js::jit::CallInfo&, js::jit::MArrayPopShift::Mode)>
-      1  <js::jit::IonBuilder::inlineDataViewGet(js::jit::CallInfo&, js::Scalar::Type)>
-      3  <js::jit::IonBuilder::inlineDataViewSet(js::jit::CallInfo&, js::Scalar::Type)>
+     10  <js::jit::IonBuilder::inlineDataViewGet(js::jit::CallInfo&, js::Scalar::Type)>
+     10  <js::jit::IonBuilder::inlineDataViewSet(js::jit::CallInfo&, js::Scalar::Type)>
       2  <js::jit::IonBuilder::inlineGetNextEntryForIterator(js::jit::CallInfo&, js::jit::MGetNextEntryForIterator::Mode)>
-      5  <js::jit::IonBuilder::inlineGuardToClass(js::jit::CallInfo&, js::jit::InlinableNative)>
+      1  <js::jit::IonBuilder::inlineGuardToClass(js::jit::CallInfo&, js::jit::InlinableNative)>
       2  <js::jit::IonBuilder::inlineIsTypedArrayHelper(js::jit::CallInfo&, js::jit::IonBuilder::WrappingBehavior)>
      19  <js::jit::IonBuilder::inlineMathFunction(js::jit::CallInfo&, js::UnaryMathFunction)>
       2  <js::jit::IonBuilder::inlineMathMinMax(js::jit::CallInfo&, bool)>
@@ -39,9 +39,9 @@
       2  <js::jit::IonBuilder::needsPostBarrier(js::jit::MDefinition*)>
       1  <js::jit::IonBuilder::newObjectTryTemplateObject(bool*, JSObject*)>
       1  <js::jit::IonBuilder::powTrySpecialized(bool*, js::jit::MDefinition*, js::jit::MDefinition*, js::jit::MIRType)>
-     11  <js::jit::IonBuilder::pushConstant(JS::Value const&)>
-      4  <js::jit::IonBuilder::pushTypeBarrier(js::jit::MDefinition*, js::TemporaryTypeSet*, js::jit::BarrierKind)>
-     20  <js::jit::IonBuilder::resumeAfter(js::jit::MInstruction*)>
+      9  <js::jit::IonBuilder::pushConstant(JS::Value const&)>
+      3  <js::jit::IonBuilder::pushTypeBarrier(js::jit::MDefinition*, js::TemporaryTypeSet*, js::jit::BarrierKind)>
+     12  <js::jit::IonBuilder::resumeAfter(js::jit::MInstruction*)>
       1  <js::jit::IonBuilder::resumeAt(js::jit::MInstruction*, unsigned char*)>
       1  <js::jit::IonBuilder::setInitializedLength(js::jit::MDefinition*, unsigned long)>
       2  <js::jit::IonBuilder::shouldAbortOnPreliminaryGroups(js::jit::MDefinition*)>
@@ -108,7 +108,7 @@
       1  <js::TemporaryTypeSet::convertDoubleElements(js::CompilerConstraintList*)>
       8  <js::TemporaryTypeSet::forAllClasses(js::CompilerConstraintList*, bool (*)(JSClass const*))>
      11  <js::TemporaryTypeSet::getKnownClass(js::CompilerConstraintList*)>
-     61  <js::TemporaryTypeSet::getKnownMIRType()>
+     60  <js::TemporaryTypeSet::getKnownMIRType()>
       1  <js::TemporaryTypeSet::getKnownRealm(js::CompilerConstraintList*)>
       5  <js::TemporaryTypeSet::getTypedArrayType(js::CompilerConstraintList*, js::TemporaryTypeSet::TypedArraySharedness*)>
       1  <js::TemporaryTypeSet::maybeSingleton()>

Interestingly, there's no change in the set of functions called, but in the number of calls emitted. Most functions are called less times, and the overall code generated in ca. 8% smaller, which seems to indicate better optimization. The only functions for which more calls are emitted than before are js::jit::IonBuilder::inlineDataViewGet and js::jit::IonBuilder::inlineDataViewSet.

Jan, what do you think about this? Is there maybe another, critical but smaller, function that it would make sense to inspect?

[1] I took the generated libxul.so binaries (libxul.so-r670435 and libxul.so-r670460 corresponding to my local hg revisions) and ran:

gdb -batch -ex 'file libxul.so-r670435' -ex "disassemble 'js::jit::IonBuilder::inlineNativeCall'"  >inlineNativeCall.before
gdb -batch -ex 'file libxul.so-r670460' -ex "disassemble 'js::jit::IonBuilder::inlineNativeCall'"  >inlineNativeCall.after
for x in inlineNativeCall.* ; do echo $a; grep callq $x | grep " <[^+].*>" -o | sort | uniq -c > $x.calls ; done
diff inlineNativeCall.before.calls inlineNativeCall.after.calls  -u
Flags: needinfo?(jdemooij)

Here, Maybe can be used instead. Also, the returned string is always a literal
string, which makes MimeResultType a trivial type now.

sizeof(mozilla::AlignedStorage2<T>) is always at least sizeof(uint64_t), which
is bad when T is bool, e.g.

Depends on D97071

(In reply to Simon Giesecke [:sg] [he/him] from comment #1)

Jan, what do you think about this? Is there maybe another, critical but smaller, function that it would make sense to inspect?

The good news is that IonBuilder is about to be removed, I have patches that should land this week. That will remove most uses of AbortReasonOr. That said, it's also used in a few places in WarpOracle.cpp, if it's easy to do the analysis on that code I'd be interested in the results.

How does this affect sizeof(AbortReasonOr<Ok>), sizeof(AbortReasonOr<bool>) and sizeof(AbortReasonOr<SomePointer*>)? Can we add a static_assert for that?

Flags: needinfo?(jdemooij)

(In reply to Jan de Mooij [:jandem] from comment #8)

(In reply to Simon Giesecke [:sg] [he/him] from comment #1)

Jan, what do you think about this? Is there maybe another, critical but smaller, function that it would make sense to inspect?

The good news is that IonBuilder is about to be removed, I have patches that should land this week. That will remove most uses of AbortReasonOr. That said, it's also used in a few places in WarpOracle.cpp, if it's easy to do the analysis on that code I'd be interested in the results.

Ah, I will check that again then! Can you add the bug as a blocker for this one?

How does this affect sizeof(AbortReasonOr<Ok>), sizeof(AbortReasonOr<bool>) and sizeof(AbortReasonOr<SomePointer*>)? Can we add a static_assert for that?

For the former two, there are already static_asserts at https://searchfox.org/mozilla-central/rev/c37038c592a352eda0f5e77dfb58c4929bf8bcd3/js/src/jit/JitContext.h#47-50, which I don't touch. The size of the latter should be sizeof(SomePointer*) + sizeof(AbortReason) now. I didn't note yet that AbortReasonOr<T*> is used as well (and alignof(T) is larger than 1). We can further improve this for this case by using the FreeLSB optimization. The AbortReason values need to be left-shifted, so that the LSB remains 0 for all (error) values, and a specialization of HasFreeLSB must be added. Then, sizeof(AbortReasonOr<T*>) will be just sizeof(T*) (and this will enable further future potential optimizations, since I plan to make Result<V, E> a trivial type for such cases).

I updated the patch with the HasFreeLSB optimization now, and added a static_assert for sizeof(AbortReasonOr<T*>).

(In reply to Simon Giesecke [:sg] [he/him] from comment #9)

Ah, I will check that again then! Can you add the bug as a blocker for this one?

Bug 1673553. I just landed the IonBuilder-removal patches on autoland but note that it's leave-open as I'll post more patches there the coming days/weeks.

(In reply to Simon Giesecke [:sg] [he/him] from comment #10)

I updated the patch with the HasFreeLSB optimization now, and added a static_assert for sizeof(AbortReasonOr<T*>).

Ah great.

Depends on: 1673553
Pushed by sgiesecke@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8179ff7588a3
Avoid using PackingStrategy::Variant fallback for MimeResultType. r=alwu
https://hg.mozilla.org/integration/autoland/rev/c8b1b0c576c2
Use std::aligned_storage_t instead of mozilla::AlignedStorage2 in PackingStrategy::NullIsOk implementation. r=emilio
https://hg.mozilla.org/integration/autoland/rev/8a1861088a72
Avoid using PackingStrategy::Variant fallback for CPUUsageWatcherError. r=nika,emilio
https://hg.mozilla.org/integration/autoland/rev/e9406e52b356
Avoid using PackingStrategy::Variant fallback for AbortReasonOr. r=jandem
https://hg.mozilla.org/integration/autoland/rev/2f47748b9581
Move PackingStrategy::Variant implementation to separate header file. r=emilio
https://hg.mozilla.org/integration/autoland/rev/140b214ccd36
Remove unused includes of Variant.h. r=andi
Pushed by malexandru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/49f2070cdd63
Fix Windows AARch64 bustage. a=bustage-fix
Regressions: 1679146
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: