Sylvestre Ledru [:Sylvestre] Reporter
	Description • 7 years ago

from /root/firefox-gcc-last/js/src/vm/Interpreter-inl.h:25,
                  from /root/firefox-gcc-last/js/src/vm/Interpreter.cpp:11:
 /root/firefox-gcc-last/js/src/vm/NativeObject-inl.h: In static member function 'static JS::Result<js::NativeObject*, JS::OOM&> js::NativeObject::create(JSContext*, js::gc::AllocKind, js::gc::InitialHeap, js::HandleShape, js::HandleObjectGroup)':
 /root/firefox-gcc-last/js/src/vm/NativeObject-inl.h:564:87: warning: 'void* memset(void*, int, size_t)' clearing an object of non-trivial type 'class js::HeapSlot'; use assignment instead [-Wclass-memaccess]
          memset(nobj->as<JSFunction>().fixedSlots(), 0, size - sizeof(js::NativeObject));

	Jason Orendorff [:jorendorff]
	Comment 1 • 7 years ago

Jeff, can you take a look? I'm not sure what will be best here. JSFunction is weird.

Surprisingly, we call memset with a variable size here, so the existing code might not be particularly fast...

	Jason Orendorff [:jorendorff]
	Comment 2 • 7 years ago

I'm always skeptical that functions this big, with this many if-statements, can possibly benefit from being inline. Really??

	Jan de Mooij [:jandem]
	Comment 3 • 7 years ago

(In reply to Jason Orendorff [:jorendorff] from comment #2)
> I'm always skeptical that functions this big, with this many if-statements,
> can possibly benefit from being inline. Really??

This one isn't MOZ_ALWAYS_INLINE so may not be inlined anyway, but a lot of the perf work I'm doing these days is making sure the right things get inlined and it can easily make a 20% or more perf difference on micro-benchmarks, even for pretty large functions...

	Jeff Walden [:Waldo] Assignee
	Comment 4 • 7 years ago

If I don't have this patch, I can't build a non-unified build at all.  (And I need to be able to do that to verify inline-function moves in subsequent patches don't create a time bomb.)

	Jeff Walden [:Waldo] Assignee
	Comment 5 • 7 years ago

The memset that has to go away, basically initializes everything in JSFunction or js::FunctionExtended that's not in NativeObject.  And it does so disregarding field types and other things.  (It's not quite really a variable memset-count -- it's one of two constant values.  Probably compiles into something constant with a branch somewhere.)  That really should live inside JSFunction, not in NativeObject, IMO.  Moving those field-inits into a JSFunction member function, that ordinarily would live in JSFunction-inl.h, requires that NativeObject::create move into JSFunction-inl.h.  (Plus all the things that depend on it, and the things that depend on them, &c.)

This is just code-move, not code-change.  I'll introduce the JSFunction::initFunctionSpecificFields once these functions have all moved over.

(This all is very ugh -- people should create objects using the actual type they want, not depending on a NativeObject::create workhorse that has to do garbage things like this beyond NativeObject's ambit -- but in the current system, this is the incrementally best change.)

	Jeff Walden [:Waldo] Assignee
	Comment 6 • 7 years ago

As I said, this is mostly very silly, and really this garbage should be initializing Rust-style -- computing the values of all the relevant fields of JSFunction or js::FunctionExtended, then writing them directly into the newly-allocated JSFunction.  But that's a bigger change cutting across a bunch more things.

Hopefully this increased use of the C++ object model, doesn't make us fall over from the fact that js::Allocate<T>() does *not* allocate a T by calling a T constructor.  Not calling a T constructor, means the compiler doesn't know there's a T there.  Which should really make all subsequent use of the T, as T, basically UB.  Treeherder of debug builds suggests we're probably okay:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef91ffd4b790805582ab2f7b955ef9362a3ac42c

Will do an opt-push as well, because that sort of failure mode is certainly a ton likelier to show up in an opt build.

	Jan de Mooij [:jandem]
	Comment 7 • 7 years ago

Personally I think it's nicer/simpler/faster to have JSFunction::create. There's some code duplication in this patch, but I think it's acceptable - if needed NativeObject::createInternal or something could be added. This passes all shell tests.

	Jeff Walden [:Waldo] Assignee
	Comment 8 • 7 years ago

Comment on attachment 8958039 [details] [diff] [review]
Alternative fix (prototype, parts stolen from Waldo's patch)

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

Try-pushes of my patches here indicate...well, I dunno if they indicate anything, exactly, but the failure count in the pushes *may* indicate reason for concern.  Who can say any more?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef91ffd4b790805582ab2f7b955ef9362a3ac42c&selectedJob=167148024
https://treeherder.mozilla.org/#/jobs?repo=try&revision=65d7d6cae9ce932c53a92eb7e959cc77ec7af0c1&selectedJob=167335328

The Windows g2 failures in that second push seem possibly most worrisome -- this push with none of the relevant patches may eventually indicate if there was something wrong with my approach:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8095e71dfb5325f4c1f0569448e656d6d84dcc87

Anyway -- basic idea looks fine to me.

::: js/src/vm/JSFunction-inl.h
@@ +130,5 @@
> +    const js::Class* clasp = group->clasp();
> +    MOZ_ASSERT(clasp->isJSFunction());
> +    MOZ_ASSERT(dynamicSlotsCount(shape->numFixedSlots(), shape->slotSpan(), clasp) == 0);
> +
> +    JSObject* obj = js::Allocate<JSObject>(cx, kind, /* nDynamicSlots = */ 0, heap, clasp);

Add a constexpr local for nDynamicSlots.

@@ +134,5 @@
> +    JSObject* obj = js::Allocate<JSObject>(cx, kind, /* nDynamicSlots = */ 0, heap, clasp);
> +    if (!obj)
> +        return cx->alreadyReportedOOM();
> +
> +    JSFunction* fun = static_cast<JSFunction*>(obj);

I might perhaps still cast to NativeObject here to do all the calls up to the initFunctionSpecificFields call, *then* do the as<JSFunction>() to cast after that point.  That makes clear that all these calls/inits up to here affect NativeObject fields, then the call affects the remaining JSFunction-specific fields.

Given there's only one caller, *and* given this caller is clearly JSFunction-centric, probably initFunctionSpecificFields can just be folded directly into this, IMO.

@@ +146,5 @@
> +    MOZ_ASSERT(shape->slotSpan() == 0);
> +
> +    fun->initFunctionSpecificFields(kind);
> +
> +    fun = SetNewObjectMetadata(cx, fun);

Assert

  MOZ_ASSERT(!clasp->shouldDelayMetadataBuilder(),
             "Function has no extra data hanging off it, that wouldn't be "
             "allocated at this point, that would require delaying the "
             "building of metadata for it");

	Jeff Walden [:Waldo] Assignee
	Comment 9 • 7 years ago

Mm, so

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8095e71dfb5325f4c1f0569448e656d6d84dcc87&selectedJob=167505146

suggests the Windows g2 failures may be unrelated to this change.  Maybe I tryservered on an unfortunate rev, don't have time to check now tho.

	Jason Orendorff [:jorendorff]
	Comment 10 • 7 years ago

Comment on attachment 8957992 [details] [diff] [review]
Properly declare JSTracer respecting JS_PUBLIC_API to avoid compile errors about visibility mismatch in non-unified builds

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

Ew

	Pulsebot
	Comment 11 • 7 years ago

Pushed by jwalden@mit.edu:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e15ed6f5914
Properly declare JSTracer respecting JS_PUBLIC_API to avoid compile errors about visibility mismatch in non-unified builds.  r=jorendorff

	Stefan Hindli [:stefan_hindli]
	Comment 12 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/6e15ed6f5914

	Jason Orendorff [:jorendorff]
	Comment 13 • 7 years ago

Comment on attachment 8958039 [details] [diff] [review]
Alternative fix (prototype, parts stolen from Waldo's patch)

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

Easy yes.

	Jason Orendorff [:jorendorff]
	Comment 14 • 7 years ago

Comment on attachment 8957994 [details] [diff] [review]
Initialize JSFunction-specific fields and potential extended slots in NativeObject::create without using memset to zero out fields that are non-trivial classes

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

(clearing r? in favor of jandem's patch)

	Jeff Walden [:Waldo] Assignee
	Comment 16 • 6 years ago

Comment on attachment 8973631 [details] [diff] [review]
Updated patch

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

The various fun-> initializations added here technically aren't C++-kosher, if I hadn't mentioned it, because we haven't instantiated values of those types at those locations.  (Just as we haven't for JSObject or JSFunction.)  But if this works, at least for now it's the best we can do.

::: js/src/vm/JSFunction-inl.h
@@ +102,5 @@
> +    const js::Class* clasp = group->clasp();
> +    MOZ_ASSERT(clasp->isJSFunction());
> +
> +    static constexpr size_t NumDynamicSlots = 0;
> +    MOZ_ASSERT(dynamicSlotsCount(shape->numFixedSlots(), shape->slotSpan(), clasp) == 0);

Not == NumDynamicSlots?

::: js/src/vm/NativeObject-inl.h
@@ +532,5 @@
>      debugCheckNewObject(group, shape, kind, heap);
>  
>      const js::Class* clasp = group->clasp();
>      MOZ_ASSERT(clasp->isNative());
> +    MOZ_ASSERT(!clasp->isJSFunction(), "Should use JSFunction::create");

Don't caps the reason-string.

	Jan de Mooij [:jandem]
	Comment 17 • 6 years ago

(In reply to Jeff Walden [:Waldo] from comment #16)
> > +    static constexpr size_t NumDynamicSlots = 0;
> > +    MOZ_ASSERT(dynamicSlotsCount(shape->numFixedSlots(), shape->slotSpan(), clasp) == 0);
> 
> Not == NumDynamicSlots?

I was thinking about doing that when I wrote this but wasn't sure about it. Maybe using NumDynamicSlots is a bit more explicit about the invariant we're asserting; I'll change it.

	Pulsebot
	Comment 18 • 6 years ago

Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/064597ea4795
Don't use memset to initialize JSFunction extended slots. Parts of this patch written by Waldo. r=jwalden

	Cristina Coroiu [:ccoroiu]
	Comment 19 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/064597ea4795