Closed
Bug 1437533
Opened 7 years ago
Closed 6 years ago
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]
Categories
(Core :: JavaScript Engine, enhancement, P2)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla62
People
(Reporter: Sylvestre, Assigned: Waldo)
References
(Blocks 1 open bug)
Details
Attachments
(4 files, 1 obsolete file)
(deleted),
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
Move a few NativeObject inlines into JSFunction-inl.h, anticipating use of a JSFunction-homed inline
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
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));
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...
Priority: -- → P2
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??
Updated•7 years ago
|
Flags: needinfo?(jwalden+bmo)
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...
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.)
Attachment #8957992 -
Flags: review?(jorendorff)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → jwalden+bmo
Status: NEW → ASSIGNED
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.)
Attachment #8957993 -
Flags: review?(jorendorff)
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.
Attachment #8957994 -
Flags: review?(jorendorff)
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.
Attachment #8958039 -
Flags: feedback?(jwalden+bmo)
Attachment #8958039 -
Flags: feedback?(jorendorff)
Updated•7 years ago
|
Attachment #8958039 -
Attachment description: Alternative fix → Alternative fix (prototype, parts stolen from Waldo's patch)
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");
Attachment #8958039 -
Flags: feedback?(jwalden+bmo) → feedback+
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(jwalden+bmo)
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.
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
Attachment #8957992 -
Flags: review?(jorendorff) → review+
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
Assignee | ||
Updated•7 years ago
|
Keywords: leave-open
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.
Attachment #8958039 -
Flags: feedback?(jorendorff) → feedback+
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)
Attachment #8957994 -
Flags: review?(jorendorff)
Updated•7 years ago
|
Attachment #8957993 -
Flags: review?(jorendorff)
Comment 15•6 years ago
|
||
Attachment #8958039 -
Attachment is obsolete: true
Attachment #8973631 -
Flags: review?(jwalden+bmo)
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.
Attachment #8973631 -
Flags: review?(jwalden+bmo) → review+
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.
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
Updated•6 years ago
|
Keywords: leave-open
Comment 19•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/064597ea4795
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Reporter | ||
Updated•6 years ago
|
status-firefox61:
--- → wontfix
You need to log in
before you can comment on or make changes to this bug.
Description
•