Closed Bug 1205054 Opened 4 years ago Closed 4 years ago

Remove isNullLike and other imprecise null checks

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: terrence, Assigned: terrence)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

TaggedProto is a tagged pointer wrapper. It should be treated just like our other tagged pointer wrappers -- Value and jsid -- and not as a special snowflake.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ce999f72bc9

This appears to have involved two things, although the try run may still disagree.

First, Rooted<TaggedProto> was storing this in the list of untagged JSObject* and then letting TraceNullableRoot's use of InternalGCMethods::isMarkable filter out tagged pointers. Of course this method doesn't actually do that -- it only does a null check. So.

Second, we newly need to handle HeapPtr<TaggedProto>. This requires a bit of new boilerplate for GCMethods, InternalGCMethods, and BarrieredBaseMixins to pass through in the obvious way. Then it needed us to handle TraceEdge and friends. This required adding a new overload for all the internal methods we were overloading already for Value and jsid. This is a ton of copy-paste and we now have 3 instances at every location -- I plan to consolidate all of this next.
It looks like making InternalGCMethods<TaggedPointer>::preBarrier depend on InternalGCMethods<JSObject*>::preBarrier led to a requirement for JSObject to be instantiated before it was defined. At least on every platform other than clang on linux. Looks like we also need to take the preBarrier for Value and jsid out of line, so I probably should have expected this.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=60d95053eb63

Split out vm/TaggedProto.h/cpp as its own file to resolve the circular requirements. Might have been able to get away with just moving the preBarrier def to cpp, but this seems clearer.
Attachment #8662031 - Flags: review?(sphink)
Comment on attachment 8662031 [details] [diff] [review]
remove_isNullLike-v1.diff

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

> Bug _ - Unify per and post barriers; NOT_REVIEWED

Should say "pre" if it's correct. (Also, I'd probably ask to split the code moving into a separate patch if I was reviewing.)
(In reply to :Ms2ger from comment #5)
> Comment on attachment 8662031 [details] [diff] [review]
> remove_isNullLike-v1.diff
> 
> Review of attachment 8662031 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> > Bug _ - Unify per and post barriers; NOT_REVIEWED
> 
> Should say "pre" if it's correct. (Also, I'd probably ask to split the code
> moving into a separate patch if I was reviewing.)

As per the title that I forgot to update: this /is/ the split patch. :-P
Blocks: 1205909
Comment on attachment 8662031 [details] [diff] [review]
remove_isNullLike-v1.diff

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

Makes sense to me, which probably means I'm fooling myself.
Attachment #8662031 - Flags: review?(sphink) → review+
https://hg.mozilla.org/mozilla-central/rev/c167178109fe
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
This broke the non-unified build (and the ARM64 simulator build). TaggedProto.cpp doesn't have any #includes.
Depends on: 1207793
Depends on: 1325406
You need to log in before you can comment on or make changes to this bug.