Andrew McCreight [:mccr8] Assignee
	Description • 11 years ago

Custom trace hooks are a huge footgun, and they are currently unused, so we should remove support for them from codegen.  For instance, you have to be careful about barriers, and if you don't set a JSClass flag, then you'll end up permanently disabling incremental GC.

I'll also add a comment to the place where we will emit the null to warn people about it.

	Andrew McCreight [:mccr8] Assignee
	Comment 2 • 11 years ago

Comment on attachment 801918 [details] [diff] [review]
remove it

try run: https://tbpl.mozilla.org/?tree=Try&rev=760c1d1023c0

	Boris Zbarsky [:bzbarsky]
	Comment 3 • 11 years ago

Comment on attachment 801918 [details] [diff] [review]
remove it

Remove the TRACE_HOOK_NAME variable too?

r=me

	Andrew McCreight [:mccr8] Assignee
	Comment 4 • 11 years ago

Ah, good catch.  I was going to get rid of it, but then saw it was used another place, but then I deleted the other place it was used...

https://hg.mozilla.org/integration/mozilla-inbound/rev/f4804e82856f

	Ryan VanderMeulen [:RyanVM]
	Comment 5 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/f4804e82856f

	Boris Zbarsky [:bzbarsky]
	Comment 6 • 11 years ago

Reopening this, since we backed out.  We'll try again once worker stuff is sane.

	Andrew McCreight [:mccr8] Assignee
	Comment 7 • 11 years ago

I was thinking I could do a more limited version that removes the pref thing, so we're only doing customTrace on workers.  I should also look at what workers are doing, as in a CC-fied setting using custom trace hooks is unorthodox.

	Andrew McCreight [:mccr8] Assignee
	Comment 8 • 11 years ago

Ah, it looks like bug 919457 separates out the notion of needing customTrace vs just being on a worker.

	Andrew McCreight [:mccr8] Assignee
	Comment 9 • 11 years ago

https://tbpl.mozilla.org/?tree=Try&rev=6bb0b6f88eeb

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 10 • 11 years ago

Comment on attachment 818518 [details] [diff] [review]
Eliminate bindings.conf setting for customTrace

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

I need a custom trace hook for bug 919885, but I guess I could bundle it into the "customWrapperManagement" thing I added.  What do you think?

	Andrew McCreight [:mccr8] Assignee
	Comment 11 • 11 years ago

Yeah, that seems reasonable enough to me.  customWrapperManagement could just imply customFinalize.  You'd probably want to add something to the comment in bindings.conf.  It seems a little less dangerous when bundled up into something that is already pretty scary. ;)

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 12 • 11 years ago

Comment on attachment 818518 [details] [diff] [review]
Eliminate bindings.conf setting for customTrace

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

r=me, even if I'm going to be grumpy about rebasing over it.

	Andrew McCreight [:mccr8] Assignee
	Comment 13 • 11 years ago

I can just wait until bug 919885 lands and figure out how to deal with it if it is going to be a pain.

	Andrew McCreight [:mccr8] Assignee
	Comment 14 • 11 years ago

This is kind of a footgun, but given that nobody seems to have ever used it aside from you, it can't be a very tempting one.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 15 • 11 years ago

Shouldn't be hard.  Should just need to add "or descriptor.customWrapperManagement" to "descriptor.nativeOwnership == 'worker'".

	Josh Matthews [:jdm]
	Comment 16 • 11 years ago

If we ever merge the Servo and Gecko codegen implementations, we're going to need to reintroduce this.

	Andrew McCreight [:mccr8] Assignee
	Comment 17 • 11 years ago

We could reintroduce it as "servoCustomTrace" or something. ;)

	Andrew McCreight [:mccr8] Assignee
	Comment 18 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/d64bea16611b

	Carsten Book [:Tomcat]
	Comment 19 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/d64bea16611b