All of the types we call this with derive from js::gc::Cell, so there is no need for this to take a void* and cast.

Comment on attachment 557934 [details] [diff] [review] Removes the reinterpret_cast by using the common base type. Do you need this checked in?

Yes. I do not have hg permissions yet. If you would like, I can re-up with r= and bug# in the text first.

Thanks, that would help.

Let me know if there is more I can do to easy the process.

Hi, nice patch. This page has a number of steps you can do to make it easier for people to commit: http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed The only thing I see that your patch is missing is the author. That link above tells you how to set that, under "Set yourself as author of the patch". If you aren't using Mercurial queues, you might want to figure that out, too. It is weird at first, but nice once you get used to it. The link to queues in the blog link gives a good introduction.

(In reply to Andrew McCreight [:mccr8] from comment #5) > Hi, nice patch. This page has a number of steps you can do to make it > easier for people to commit: > http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed > > The only thing I see that your patch is missing is the author. Huh, I thought that "# User Terrence Cole <terrence@trainedmonkeystudios.org>" was the author bits. :-) I had qnew = -Ue in my .hgrc, I've changed that to qnew = -U -e. What should I look for to see if this starts working? > That link > above tells you how to set that, under "Set yourself as author of the > patch". If you aren't using Mercurial queues, you might want to figure that > out, too. It is weird at first, but nice once you get used to it. The link > to queues in the blog link gives a good introduction. You are quite right. Mq took a few days to get the hang of, but is a definite "full of win" once you start groking it.

Oops, sorry! I think I wasn't looking at the right view of the patch. So it is fine.

Backed out of inbound, since either this or bug 684110, caused bustage: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&usebuildbot=1&rev=cd1957f6628d https://tbpl.mozilla.org/?tree=Mozilla-Inbound&usebuildbot=1&rev=e1ad65d6a7fd However, no way to tell which since this was pushed within the 3 minute cut-off for including in the same build cycle (see bug 684436 for stopping doing this, since it's unhelpful at best). Ideally please wait more than 3 mins since the last push before pushing the next one, or the sheriffs end up having to back a load of unnecessary changesets out. Also Bill, please may you set the target milestone when landing on m-i, per the recently updated https://wiki.mozilla.org/Inbound_Sheriff_Duty Thanks! :-)

Sorry, I missed a problem here. The problem is that we can't include jsxml.h from jsgc.h, for the following reasons. Code outside the JS engine sometimes includes our non-public header files, like jsgc.h. (We're trying to stop this from happening.) The files that can be included outside JS are listed in a variable called INSTALLED_HEADERS in the Makefile. If some other file is included, even transitively, the build fails. The problem is that jsgc.h is in INSTALLED_HEADERS, but jsxml.h is not. So if we include jsxml.h in jsgc.h, the build fails. The wrong way to fix this would be to add jsxml.h to INSTALLED_HEADERS. A better solution is to just remove the js_TraceXML prototype (and the jsxml.h include). I don't think it's needed. Could you post an updated patch? I'll push it to tryserver this time to make sure it works.

(In reply to Bill McCloskey (:billm) from comment #9) > Sorry, I missed a problem here. The problem is that we can't include jsxml.h > from jsgc.h, for the following reasons. > > Code outside the JS engine sometimes includes our non-public header files, > like jsgc.h. (We're trying to stop this from happening.) The files that can > be included outside JS are listed in a variable called INSTALLED_HEADERS in > the Makefile. If some other file is included, even transitively, the build > fails. The problem is that jsgc.h is in INSTALLED_HEADERS, but jsxml.h is > not. So if we include jsxml.h in jsgc.h, the build fails. I think I follow. > The wrong way to fix this would be to add jsxml.h to INSTALLED_HEADERS. A > better solution is to just remove the js_TraceXML prototype (and the jsxml.h > include). I don't think it's needed. My intention was certainly not to change the public api! We shouldn't need jsxml.h, but we will need some other changes if we remove it. If it isn't present, the best definition the compiler has for JSXML is "typedef struct JSXML JSXML;" in jsprvtd.h. Without a real definition of JSXML, the compiler does not know that JSXML is a js::gc::Cell and thus complains about not having a delayMarkingChildren it can call. We should be able to move the reinterpret_cast to be only on the JSXML object in this case, which would be a fair compromise, I think. Possibly a better solution would be to move the definitions of the callers (pushXML, et.al.) to jsgc.cpp where (please correct me if I am wrong) we can import jsxml.h safely. > Could you post an updated patch? I'll push it to tryserver this time to make > sure it works. I'll whip something up -- hopefully not stepping on any landmines this time -- as soon as time allows.

This should be equivalent, but is now tested to build on both the js shell and the full mozilla-central repository.

Unfortunately, we can't do it this way. The pushX functions are super-hot, so they really need to be inline. We could move them to jsgcinline.h or jsgcmark.cpp, but that seems kinda hacky. I think it's probably best to wait for bug 677079 to land. That will allow us to include jsxml.h in jsgc.h, which is really the right solution.

(In reply to Bill McCloskey (:billm) from comment #12) > Unfortunately, we can't do it this way. The pushX functions are super-hot, > so they really need to be inline. We could move them to jsgcinline.h or > jsgcmark.cpp, but that seems kinda hacky. I didn't think of that. Out of curiosity, I ran sunspider to see how much carnage I had wrought. Results are: Optimized: FROM: 238.5ms +/- 0.6% TO: 238.4ms +/- 0.4% Debug: FROM: 1724.7ms +/- 0.1% TO: 1724.9ms +/- 0.2% Looks like g++ is smart enough to inline these, even in the debug case, even without hints from us mere developers. How well does JS_ALWAYS_INLINE work, in the real world? If that's an option, I think it could serve equally well as documentation. > I think it's probably best to wait for bug 677079 to land. That will allow > us to include jsxml.h in jsgc.h, which is really the right solution.

Does GC actually run during Sunspider? I thought a key tenet of GC tuning was to make it run as little as possible during Sunspider. ;) V8 involves more GC overhead, I think.

Yes, we shouldn't be GCing during the timed portion of SunSpider. V8 would be a better benchmark. Even better would be something like this: for (var i=0; i<10000; i++) gc(); I would be very surprised if you don't see a difference there. JS_ALWAYS_INLINE should only affect the compiler's inlining heuristics. So if it decided that a function was too big to inline, then JS_ALWAYS_INLINE can override that. However, it won't allow functions to be inlined across translation units (i.e., .cpp files). You need link-time optimization for that, which I think is not part of our build process. It's a pretty new part of gcc, I think.

Doh! The GC not running would rather explain these benchmarks. :-) Here is v8 on an optimized build: FROM: 1297.5ms +/- 0.1% TO: 1295.6ms +/- 0.1% Same result. Working smarter this time, I decided to just look in the binary to see if the functions are inlined: they are not. I had not considered the problems with inlining across translation units, but it's obvious in hindsight. I still want to know what the performance costs are: I'll try a microbenchmark next.

Performance on the microbenchmark [:billm] suggested: head: 5.69sec new: 6.27sec It is a bigger impact than I was expecting. Thanks for the help tracking down what was going on!

Can we land this?

As per comment 17, no.