The PassRefPtr ref-churn optimization really does result in some performance wins. As one data point, in working the parser, it was very easy to see the performance benefits of using PassRefPtr in a couple of specific parameters. Those were cases where the callers already had a PassRefPtr returned from another function.
I'd be in favor of (1). Even if the performance optimization doesn't matter in every case, it's still helpful to teach contributors how these classes work so that they can apply them correctly in the cases where they do matter. It's a bit like exercising our collective PassRefPtr muscles so they're ready when need to do some heavy lifting. Adam On Sat, Jun 18, 2011 at 10:58 PM, Maciej Stachowiak <m...@apple.com> wrote: > > On Jun 18, 2011, at 10:47 PM, Alexey Proskuryakov wrote: > >> >> 18.06.2011, в 22:15, Maciej Stachowiak написал(а): >> >>> - I think having a rule for using PassRefPtr for arguments that depends on >>> how callers use the function is likely to be hard to use in practice, since >>> it requires global knowledge of all callers to pick the right function >>> signature. The rule I prefer is that if a function takes ownership of the >>> argument, whether or not we currently have a caller that gives away >>> ownership, the function should take PassRefPtr. If it does not take >>> ownership, it should take a raw pointer. That way you can decide the right >>> thing to do based solely on the definition of the function itself. >> >> >> Using PassRefPtr for arguments is a fairly common source of bugs. The way >> it's zeroed out after being used has caused crashes in code paths that >> weren't tested before commit for some reason (see e.g. >> <https://bugs.webkit.org/show_bug.cgi?id=52981>). Another example that we've >> just seen (<https://bugs.webkit.org/show_bug.cgi?id=62836>) was when a >> different compiler had a different order of evaluation, so building with it >> suddenly exposed a crash that we didn't need to have. >> >> Looking at either bug, using PassRefPtr is pointless. For trace(), we could >> theoretically pass ownership from jsConsolePrototypeFunctionTrace(), but we >> do not, and micro-optimizing Console functions for performance isn't worth >> the cost of having had this crasher bug in my opinion. For bug 62836, it's >> quite clearly impossible to pass ownership to HTMLTableRowsCollection. > > Yes, the self-zeroing can cause bugs. But passing raw pointers can also cause > bugs, and possibly more serious ones because they'll be freed memory access > instead of null pointer derefs. I can see three options: > > (1) Use PassRefPtr for every parameter that takes ownership. > > Pro: > Bright-line rule; fairly clear how to apply it. > Distributed performance benefit. > > Con: > Possible accidental self-zeroing bugs. > Maybe slight code bloat if it leads to reefing at call sites. > > (2) Abandon PassRefPtr for incoming function arguments entirely. > > Pro: > Extremely simple to apply the rule. > No accidental-self-zeroing bugs. > > Con: > We probably lose some performance. > Possible accidental freed memory access bugs. > For cases where the object being passed really is freshly created, now > you have to make a local RefPtr variable to hold it. > > (3) Use PassRefPtr based on whether callers ever provide a PassRefPtr as an > argument. > > Pro: > If done right, get the performance benefit without the costs/hazards. > > Con: > Applying this rule is hard; coding or reviewing a patch that changes > PassRefPtrness of an argument in principle requires checking every call site. > If people apply the rule wrong, we can get both null-deref and > freed-memory bugs. > > (3) is my least favorite. > > Maybe there are other options or other pros and cons. It might be useful to > make a wiki page if we want to collect more options and arguments. > > Regards, > Maciej > > _______________________________________________ > webkit-dev mailing list > webkit-dev@lists.webkit.org > http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev > _______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
