Re: [webkit-dev] Lets use PassRefPtr for arguments less; lets use RefPtr for locals and data members more
20.06.2011, в 21:55, David Levin написал(а): PassRefPtr is useful even if the function doesn't take any kind of ownership. It's useful when callers want to get rid of ownership, and then they can do that efficiently. I don't understand this. How it is more efficient to pass ownership if a function isn't taking ownership? It seems like passing in a raw pointer would be the most efficient in this case. As a concrete example, consider a callback function. It can take a reference to an object as an argument - the caller definitely no longer needs this object after calling it, while the function may or may not take ownership (in any sense of this word). In an extreme case, it will not look at its argument at all. Similarly, a base class implementation of a virtual member function may not do anything with its argument, while derived objects will take ownership. It's not easy to tell if any of the overrides do (or will in the future) take ownership. Yet it's easy to see if ownership is being passed away at call sites. - WBR, Alexey Proskuryakov ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Lets use PassRefPtr for arguments less; lets use RefPtr for locals and data members more
On Jun 19, 2011, at 12:48 PM, Darin Adler wrote: The arguments about abandoning PassRefPtr for arguments entirely are attacking a straw man. We know PassRefPtr offers an important optimization and do not want to drop that! I posited it not as a straw man but rather as an option I myself put on the table. Seems most folks agree it is a bad one. Also, examining the pros and cons of using PassRefPtr in all the cases where we do now vs in none of the cases where we do now sheds light on the relative advantages and disadvantages of using it only in some of the cases where we do now. Note that I listed using PassRefPtr for arguments less often (but not never) as a separate option. On Jun 18, 2011, at 10:58 PM, Maciej Stachowiak wrote: (1) Use PassRefPtr for every parameter that takes ownership. I still think this is the appropriate rule, and always have, but I think “takes ownership” is not defined to my satisfaction. Pro (of using PassRefPtr for every parameter that takes ownership): Bright-line rule; fairly clear how to apply it. This is where my problem comes in. I am not sure what the bright-line rule is. It's approximately this function will put the relevant parameter in a RefPtr data member or global. The DOM is more complicated, because much of the internal refcounting is done manually instead of using RefPtrs. Generally, in the DOM, the rule is that all reachable objects need to be kept alive, so reference counting is used to implement reachability, which does not match with my sense of the word ownership. Reference-counted objects have shared ownership. Every object holding a reference to any other owns it. We could stop using the word ownership and instead of take ownership say take a persistent reference. For a shared ownership model there are multiple possible definitions of whether a function takes ownership to an object passed as an argument. Here are some of my attempts to describe the bright line: a) Hands off ownership to what could possibly be the sole owner in most code paths. b) Keeps a reference to the object after the function completes in most code paths. c) Takes a reference to the object at least once in most code paths. d) Hands off ownership to what could possibly be the sole owner in some code paths. e) Keeps a reference to the object after it completes in some code paths. f) Takes a reference to the object at least once in some code paths. Is the bright line rule you have in mind (b) or perhaps (e)? Or something not listed here at all? Yes, (b) or (e). I haven't thought about whether most code paths or some code paths makes more sense, but I'm not sure there are a lot of cases in our code where it makes a difference. I don't think it makes sense to talk about a sole owner of a refcounted object. If there is a sole owner at any given time, it is at best temporary. PassRefPtr is about clearly and efficiently transferring a reference, it doesn't need it to necessarily be the sole then-existing reference. […snip…] Similarly, some functions call out to operations that could cause the last owner for one of their arguments to dereference the object. They are likely to fit into category (c) or (f). In a sense the caller does have to pass ownership of the object, because the operation can only be successfully done if the function takes ownership to make sure the object does not disappear while the code is working with it. My understanding is that our current rule doesn't call for taking a PassRefPtr argument in this case, but I can see how it would make sense and would add helpful type checking. I tried to find the examples that bother me. Here are some DOM examples where the term ownership seems wrong, and it’s more about reachability: HTMLCollection::create: Does a collection own the node it is rooted in? MessageEvent::initMessageEvent: Does a message event own the source DOM window? Range::create: Does a range own the document its nodes are in and the nodes that are used to specify the endpoints? Storage::create: Does a storage object own its storage area? UIEvent::create: Does an event own its abstract view? It seems to me that all of these objects become shared-owners of the respective parameter, in the sense of holding a persistent reference. Some don't literally stick it in a RefPtr but they do the moral equivalent. Here are some examples that are not purely DOM reachability that I find confusing: Document::setTitleElement: Does a document own its title element? FrameLoaderClient::dispatchWillSubmitForm: Does communication of a pending form submission involve passing ownership of form state? Ditto. Regards, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Lets use PassRefPtr for arguments less; lets use RefPtr for locals and data members more
20.06.2011, в 03:22, Maciej Stachowiak написал(а): For a shared ownership model there are multiple possible definitions of whether a function takes ownership to an object passed as an argument. Here are some of my attempts to describe the bright line: a) Hands off ownership to what could possibly be the sole owner in most code paths. b) Keeps a reference to the object after the function completes in most code paths. c) Takes a reference to the object at least once in most code paths. d) Hands off ownership to what could possibly be the sole owner in some code paths. e) Keeps a reference to the object after it completes in some code paths. f) Takes a reference to the object at least once in some code paths. Is the bright line rule you have in mind (b) or perhaps (e)? Or something not listed here at all? Yes, (b) or (e). I haven't thought about whether most code paths or some code paths makes more sense, but I'm not sure there are a lot of cases in our code where it makes a difference. I don't think it makes sense to talk about a sole owner of a refcounted object. If there is a sole owner at any given time, it is at best temporary. PassRefPtr is about clearly and efficiently transferring a reference, it doesn't need it to necessarily be the sole then-existing reference. I think that to make this complete, the rules need to be transitive. A function that passes its argument to another function taking a PassRefPtr should itself take a PassRefPtr. That's the case in https://bugs.webkit.org/show_bug.cgi?id=52981, for instance. It doesn't seem that analyzing code for most/some code paths takes ownership would be any easier that analyzing it for any caller gives away ownership. Yet it's the latter where PassRefPtr is beneficial. Why base the rule on something that's disconnected from actual benefit? - WBR, Alexey Proskuryakov ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Lets use PassRefPtr for arguments less; lets use RefPtr for locals and data members more
On Mon, Jun 20, 2011 at 9:19 AM, Alexey Proskuryakov a...@webkit.org wrote: I think that to make this complete, the rules need to be transitive. A function that passes its argument to another function taking a PassRefPtr should itself take a PassRefPtr. That's the case in https://bugs.webkit.org/show_bug.cgi?id=52981, for instance. I agree. Most of editing bugs come from breaking that transitive rule. I even want it be enforced by some C++ idiom; e.g. replace all raw pointers in argument list by PassRawPtr, which cannot be converted to PassRefPtr or RefPtr. - Ryosuke ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Lets use PassRefPtr for arguments less; lets use RefPtr for locals and data members more
On Jun 20, 2011, at 9:19 AM, Alexey Proskuryakov wrote: 20.06.2011, в 03:22, Maciej Stachowiak написал(а): For a shared ownership model there are multiple possible definitions of whether a function takes ownership to an object passed as an argument. Here are some of my attempts to describe the bright line: a) Hands off ownership to what could possibly be the sole owner in most code paths. b) Keeps a reference to the object after the function completes in most code paths. c) Takes a reference to the object at least once in most code paths. d) Hands off ownership to what could possibly be the sole owner in some code paths. e) Keeps a reference to the object after it completes in some code paths. f) Takes a reference to the object at least once in some code paths. Is the bright line rule you have in mind (b) or perhaps (e)? Or something not listed here at all? Yes, (b) or (e). I haven't thought about whether most code paths or some code paths makes more sense, but I'm not sure there are a lot of cases in our code where it makes a difference. I don't think it makes sense to talk about a sole owner of a refcounted object. If there is a sole owner at any given time, it is at best temporary. PassRefPtr is about clearly and efficiently transferring a reference, it doesn't need it to necessarily be the sole then-existing reference. I think that to make this complete, the rules need to be transitive. A function that passes its argument to another function taking a PassRefPtr should itself take a PassRefPtr. That's the case in https://bugs.webkit.org/show_bug.cgi?id=52981, for instance. It doesn't seem that analyzing code for most/some code paths takes ownership would be any easier that analyzing it for any caller gives away ownership. In general it only requires inspecting the source of the function, and possibly the signatures of function it calls. The analysis for any caller gives away ownership requires searching over all of WebCore to find possible call sites. And it can easily change If we think the transitivity problem is a hazard, we could make PassRefPtr refuse to implicitly convert from raw pointers. Then to pass a raw pointer to a function that uses PassRefPtr you'd have to make a RefPtr first (or adopt, if the ref is unowned). That would make takes ownership analysis purely local to the function source. Yet it's the latter where PassRefPtr is beneficial. Why base the rule on something that's disconnected from actual benefit? Because it's simpler to read the source of your own function than to visit all call sites, and it's more obvious that when you change what the function does you may need to change the signature. Regards, Macij ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Lets use PassRefPtr for arguments less; lets use RefPtr for locals and data members more
On Mon, Jun 20, 2011 at 5:25 PM, Maciej Stachowiak m...@apple.com wrote: On Jun 20, 2011, at 9:19 AM, Alexey Proskuryakov wrote: 20.06.2011, в 03:22, Maciej Stachowiak написал(а): For a shared ownership model there are multiple possible definitions of whether a function takes ownership to an object passed as an argument. Here are some of my attempts to describe the bright line: a) Hands off ownership to what could possibly be the sole owner in most code paths. b) Keeps a reference to the object after the function completes in most code paths. c) Takes a reference to the object at least once in most code paths. d) Hands off ownership to what could possibly be the sole owner in some code paths. e) Keeps a reference to the object after it completes in some code paths. f) Takes a reference to the object at least once in some code paths. Is the bright line rule you have in mind (b) or perhaps (e)? Or something not listed here at all? Yes, (b) or (e). I haven't thought about whether most code paths or some code paths makes more sense, but I'm not sure there are a lot of cases in our code where it makes a difference. I don't think it makes sense to talk about a sole owner of a refcounted object. If there is a sole owner at any given time, it is at best temporary. PassRefPtr is about clearly and efficiently transferring a reference, it doesn't need it to necessarily be the sole then-existing reference. I think that to make this complete, the rules need to be transitive. A function that passes its argument to another function taking a PassRefPtr should itself take a PassRefPtr. That's the case in https://bugs.webkit.org/show_bug.cgi?id=52981, for instance. It doesn't seem that analyzing code for most/some code paths takes ownership would be any easier that analyzing it for any caller gives away ownership. In general it only requires inspecting the source of the function, and possibly the signatures of function it calls. The analysis for any caller gives away ownership requires searching over all of WebCore to find possible call sites. And it can easily change If we think the transitivity problem is a hazard, we could make PassRefPtr refuse to implicitly convert from raw pointers. Then to pass a raw pointer to a function that uses PassRefPtr you'd have to make a RefPtr first (or adopt, if the ref is unowned). That would make takes ownership analysis purely local to the function source. I thought the problem was that a Pass*Ptr could silently 0 itself out and people use it directly. So the real problem functions are anything the fact that a Pass*Ptr can be used like a pointer and that another Pass*Ptr can slurp out its contents so easily (and you can't spot this in the code well). Maybe each of those operations should be more explicit? (Then we could probably even catch these issues automatically. If *.passPtr() is called and then *.get() is called later in the same function, mark it as an error. Not perfect but good enough.) Yet it's the latter where PassRefPtr is beneficial. Why base the rule on something that's disconnected from actual benefit? Because it's simpler to read the source of your own function than to visit all call sites, and it's more obvious that when you change what the function does you may need to change the signature. This seems much easier, less error prone, and more stable. dave Regards, Macij ___ 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
Re: [webkit-dev] Lets use PassRefPtr for arguments less; lets use RefPtr for locals and data members more
20.06.2011, в 17:25, Maciej Stachowiak написал(а): Yet it's the latter where PassRefPtr is beneficial. Why base the rule on something that's disconnected from actual benefit? Because it's simpler to read the source of your own function than to visit all call sites, and it's more obvious that when you change what the function does you may need to change the signature. I do not see how you are describing a practical coding situation here. I do not start with a dozen call sites all over the code base, and then write a function they all call. On the other hand, when adding a new call site that wants to pass ownership away, and the called function doesn't take a PassRefPtr, it's immediately obvious that it's not going to work. Even when following a cargo cult rule is easier (not uncommon!), the problem of it being disconnected from the actual benefit still remains. - WBR, Alexey Proskuryakov ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Lets use PassRefPtr for arguments less; lets use RefPtr for locals and data members more
On Mon, Jun 20, 2011 at 6:11 PM, Alexey Proskuryakov a...@webkit.org wrote: 20.06.2011, в 17:25, Maciej Stachowiak написал(а): Yet it's the latter where PassRefPtr is beneficial. Why base the rule on something that's disconnected from actual benefit? Because it's simpler to read the source of your own function than to visit all call sites, and it's more obvious that when you change what the function does you may need to change the signature. I do not see how you are describing a practical coding situation here. I do not start with a dozen call sites all over the code base, and then write a function they all call. On the other hand, when adding a new call site that wants to pass ownership away, and the called function doesn't take a PassRefPtr, it's immediately obvious that it's not going to work. Even when following a cargo cult rule is easier (not uncommon!), the problem of it being disconnected from the actual benefit still remains. Here's a few benefits: 1. It makes the code more self-documenting. It clearly indicates that this function intends to take a reference to the item. 2. It is consistent with the rules for PassOwnPtr. It is nice to have one set of things in mind that are consistent. 3. Just like one shouldn't document a function based on who calls it because that may change, it makes sense to base the argument types on the how the function uses them not on the callers. - WBR, Alexey Proskuryakov ___ 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
Re: [webkit-dev] Lets use PassRefPtr for arguments less; lets use RefPtr for locals and data members more
20.06.2011, в 21:29, David Levin написал(а): Here's a few benefits: 1. It makes the code more self-documenting. It clearly indicates that this function intends to take a reference to the item. 2. It is consistent with the rules for PassOwnPtr. It is nice to have one set of things in mind that are consistent. 3. Just like one shouldn't document a function based on who calls it because that may change, it makes sense to base the argument types on the how the function uses them not on the callers. 1 and 3 sound very closely related. Yet I'm not sure if they are good. PassRefPtr is useful even if the function doesn't take any kind of ownership. It's useful when callers want to get rid of ownership, and then they can do that efficiently. As Maciej mentioned, eventually we may be able to get rid of PassRefPtr, and use C++0x move semantics. But applicability of move semantics also depends on how a function is called, not on what it is going to do with its arguments. - WBR, Alexey Proskuryakov ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Lets use PassRefPtr for arguments less; lets use RefPtr for locals and data members more
On Mon, Jun 20, 2011 at 9:39 PM, Alexey Proskuryakov a...@webkit.org wrote: 20.06.2011, в 21:29, David Levin написал(а): Here's a few benefits: 1. It makes the code more self-documenting. It clearly indicates that this function intends to take a reference to the item. 2. It is consistent with the rules for PassOwnPtr. It is nice to have one set of things in mind that are consistent. 3. Just like one shouldn't document a function based on who calls it because that may change, it makes sense to base the argument types on the how the function uses them not on the callers. 1 and 3 sound very closely related. Yet I'm not sure if they are good. True, I didn't think of it that way when I wrote it. :) I feel like they are good, but I could be wrong. (I always have felt like knowing who is owning something is one of those tricky things when reading code so I've appreciated this but it may be of limited value when taking about ref counted items.) And I like 2 but it isn't critical. PassRefPtr is useful even if the function doesn't take any kind of ownership. It's useful when callers want to get rid of ownership, and then they can do that efficiently. I don't understand this. How it is more efficient to pass ownership if a function isn't taking ownership? It seems like passing in a raw pointer would be the most efficient in this case. As Maciej mentioned, eventually we may be able to get rid of PassRefPtr, and use C++0x move semantics. But applicability of move semantics also depends on how a function is called, not on what it is going to do with its arguments. Good point. Makes sense. dave - WBR, Alexey Proskuryakov ___ 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
Re: [webkit-dev] Lets use PassRefPtr for arguments less; lets use RefPtr for locals and data members more
The arguments about abandoning PassRefPtr for arguments entirely are attacking a straw man. We know PassRefPtr offers an important optimization and do not want to drop that! On Jun 18, 2011, at 10:58 PM, Maciej Stachowiak wrote: (1) Use PassRefPtr for every parameter that takes ownership. I still think this is the appropriate rule, and always have, but I think “takes ownership” is not defined to my satisfaction. Pro (of using PassRefPtr for every parameter that takes ownership): Bright-line rule; fairly clear how to apply it. This is where my problem comes in. I am not sure what the bright-line rule is. Generally, in the DOM, the rule is that all reachable objects need to be kept alive, so reference counting is used to implement reachability, which does not match with my sense of the word ownership. For a shared ownership model there are multiple possible definitions of whether a function takes ownership to an object passed as an argument. Here are some of my attempts to describe the bright line: a) Hands off ownership to what could possibly be the sole owner in most code paths. b) Keeps a reference to the object after the function completes in most code paths. c) Takes a reference to the object at least once in most code paths. d) Hands off ownership to what could possibly be the sole owner in some code paths. e) Keeps a reference to the object after it completes in some code paths. f) Takes a reference to the object at least once in some code paths. Is the bright line rule you have in mind (b) or perhaps (e)? Or something not listed here at all? Editing operations that are undoable often meet (b) but not (a). It’s confusing to me that editing operations want to “take ownership” of arguments. To make editing undoable the undo machinery may have to reference these objects, but it may also have to reference objects as well, perhaps the document or other elements. It’s not clear to me that requesting editing by passing arguments that specify where in the document the edit should take place means that the function takes ownership of those arguments. If we tell the document which is the currently active title element by calling setTitleElement, the function meets (b) but not (a). Similarly, some functions call out to operations that could cause the last owner for one of their arguments to dereference the object. They are likely to fit into category (c) or (f). In a sense the caller does have to pass ownership of the object, because the operation can only be successfully done if the function takes ownership to make sure the object does not disappear while the code is working with it. Con (of using PassRefPtr for every parameter that takes ownership): Possible accidental self-zeroing bugs. I think the “possible” adjective here makes this sound like a smaller problem than it is. Alexey and I have both seen multiple examples of this, particularly in contexts like constructor initialization lists where the prp idiom does not work well. Con (of abandoning PassRefPtr for function arguments entirely): Possible accidental freed memory access bugs. I think the reverse of this Con is one of the stronger Pro arguments for using PassRefPtr even more for arguments rather than my proposal to use it less. Object lifetime mistakes are much less likely when raw pointers are kept to an absolute minimum. I thought about this when reviewing the design of Automatic Reference Counting. The ARC design largely eliminates raw pointers for Objective-C objects. I tried to find the examples that bother me. Here are some DOM examples where the term ownership seems wrong, and it’s more about reachability: HTMLCollection::create: Does a collection own the node it is rooted in? MessageEvent::initMessageEvent: Does a message event own the source DOM window? Range::create: Does a range own the document its nodes are in and the nodes that are used to specify the endpoints? Storage::create: Does a storage object own its storage area? UIEvent::create: Does an event own its abstract view? Here are some examples that are not purely DOM reachability that I find confusing: Document::setTitleElement: Does a document own its title element? FrameLoaderClient::dispatchWillSubmitForm: Does communication of a pending form submission involve passing ownership of form state? I couldn’t quickly find other examples, but I have a vague sense that there are some that are even more confusing. -- Darin ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Lets use PassRefPtr for arguments less; lets use RefPtr for locals and data members more
On Sun, Jun 19, 2011 at 12:48 PM, Darin Adler da...@apple.com wrote: Con (of abandoning PassRefPtr for function arguments entirely): Possible accidental freed memory access bugs. I think the reverse of this Con is one of the stronger Pro arguments for using PassRefPtr even more for arguments rather than my proposal to use it less. Object lifetime mistakes are much less likely when raw pointers are kept to an absolute minimum. I thought about this when reviewing the design of Automatic Reference Counting. The ARC design largely eliminates raw pointers for Objective-C objects. One of the most common security bugs I have seen in editing is that we keep a raw pointer to a node and call some helper method that modifies DOM (therefore invoking scripts). I'm sometimes tempted to replace all instances of Node* in the editing component by RefPtr/PassRefPtr. - Ryosuke ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Lets use PassRefPtr for arguments less; lets use RefPtr for locals and data members more
On Jun 19, 2011, at 2:03 PM, Ryosuke Niwa wrote: One of the most common security bugs I have seen in editing is that we keep a raw pointer to a node and call some helper method that modifies DOM (therefore invoking scripts). I'm sometimes tempted to replace all instances of Node* in the editing component by RefPtr/PassRefPtr. I suspect that if the data members and local variables had type RefPtr, then it mostly wouldn’t matter if argument types were PassRefPtr or raw pointers for this purpose. -- Darin ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Lets use PassRefPtr for arguments less; lets use RefPtr for locals and data members more
On Sun, Jun 19, 2011 at 2:05 PM, Darin Adler da...@apple.com wrote: On Jun 19, 2011, at 2:03 PM, Ryosuke Niwa wrote: One of the most common security bugs I have seen in editing is that we keep a raw pointer to a node and call some helper method that modifies DOM (therefore invoking scripts). I'm sometimes tempted to replace all instances of Node* in the editing component by RefPtr/PassRefPtr. I suspect that if the data members and local variables had type RefPtr, then it mostly wouldn’t matter if argument types were PassRefPtr or raw pointers for this purpose. Right, although it's tricky to catch cases where we call a function that takes multiple arguments (one of them being Node*) with the return value of a function that modifies DOM. - Ryosuke ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
[webkit-dev] Lets use PassRefPtr for arguments less; lets use RefPtr for locals and data members more
1: Recently, Alexey has encouraged me to use PassRefPtr less for function arguments. The PassRefPtr optimization pays off when the object in question is possibly being handed off from one PassRefPtr to another. For an argument, that can happen in two ways: 1) The argument can be the result of a function that returns a PassRefPtr. 2) The argument can be the result of calling release a local or data member that is a RefPtr. In both of those cases, we are transferring ownership. Mechanically speaking, PassRefPtr only pays off if we are actually getting that optimization. If we are passing a raw pointer, then using PassRefPtr for the function argument type doesn’t help much. It just puts a ref at the call site, a ref that otherwise would happen inside the function. It may even cause a bit of code bloat if there is more than one call site. Conceptually speaking, PassRefPtr only pays off if the context is a clear transfer of ownership. Passing an object that the recipient *might* later choose to take shared ownership of is not enough. Clients are always welcome to take shared ownership of something passed with a raw pointer. Because there are also costs to PassRefPtr, we should reserve PassRefPtr arguments for cases where the optimization will really pay off and for where the function definitely is taking ownership. Those functions do exist, but many current uses of PassRefPtr for arguments do not qualify. 2: Recently, I’ve noticed that many bugs simply would cease to exist if we used RefPtr more and raw pointer less for things like local variables and data members. The time it’s safe to use a raw pointer for a local variable or data member for a reference counted object pointer is when there is some guarantee that someone else is holding a reference. It can be difficult to have such a guarantee and those guarantees are fragile as code executes. Nowhere is that more clear than in loader-related code. Some are loathe to use RefPtr for data members because they are concerned about reference cycles. Generally speaking, we can eliminate the worry about reference cycles by making destruction include a “closing” process, which can null out all the references and break cycles. If we find it’s necessary we can also look into an efficient WeakPtr implementation. Some have been enthusiastic about this in the past. I have been a bit less so because I don’t know of an efficient implementation strategy. Conclusion: A specific example where an argument has type PassRefPtr, but that does not seem like the correct design, is the the node argument in the constructors of the various HTMLCollection classes. Examples of where we are using raw pointers, but should use RefPtr instead abound. Sadly there is no way to qualify a member function to make “this” a RefPtr, so we are stuck with the “protector” idiom for the this pointer. Maybe we can even find a way to discuss these two issues in the RefPtr document without making it too confusing. Comments are welcome. But lets not bikeshed http://en.wikipedia.org/wiki/Parkinson's_Law_of_Triviality this! -- Darin ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Lets use PassRefPtr for arguments less; lets use RefPtr for locals and data members more
I've fixed many a security bug cause by not refing local variables. Generally, you need to ref your local variables if any function you call can end up executing JavaScript (and many can). If you're not sure, please feel encouraged to ask around. Adam On Jun 18, 2011 8:54 PM, Darin Adler da...@apple.com wrote: 1: Recently, Alexey has encouraged me to use PassRefPtr less for function arguments. The PassRefPtr optimization pays off when the object in question is possibly being handed off from one PassRefPtr to another. For an argument, that can happen in two ways: 1) The argument can be the result of a function that returns a PassRefPtr. 2) The argument can be the result of calling release a local or data member that is a RefPtr. In both of those cases, we are transferring ownership. Mechanically speaking, PassRefPtr only pays off if we are actually getting that optimization. If we are passing a raw pointer, then using PassRefPtr for the function argument type doesn’t help much. It just puts a ref at the call site, a ref that otherwise would happen inside the function. It may even cause a bit of code bloat if there is more than one call site. Conceptually speaking, PassRefPtr only pays off if the context is a clear transfer of ownership. Passing an object that the recipient *might* later choose to take shared ownership of is not enough. Clients are always welcome to take shared ownership of something passed with a raw pointer. Because there are also costs to PassRefPtr, we should reserve PassRefPtr arguments for cases where the optimization will really pay off and for where the function definitely is taking ownership. Those functions do exist, but many current uses of PassRefPtr for arguments do not qualify. 2: Recently, I’ve noticed that many bugs simply would cease to exist if we used RefPtr more and raw pointer less for things like local variables and data members. The time it’s safe to use a raw pointer for a local variable or data member for a reference counted object pointer is when there is some guarantee that someone else is holding a reference. It can be difficult to have such a guarantee and those guarantees are fragile as code executes. Nowhere is that more clear than in loader-related code. Some are loathe to use RefPtr for data members because they are concerned about reference cycles. Generally speaking, we can eliminate the worry about reference cycles by making destruction include a “closing” process, which can null out all the references and break cycles. If we find it’s necessary we can also look into an efficient WeakPtr implementation. Some have been enthusiastic about this in the past. I have been a bit less so because I don’t know of an efficient implementation strategy. Conclusion: A specific example where an argument has type PassRefPtr, but that does not seem like the correct design, is the the node argument in the constructors of the various HTMLCollection classes. Examples of where we are using raw pointers, but should use RefPtr instead abound. Sadly there is no way to qualify a member function to make “this” a RefPtr, so we are stuck with the “protector” idiom for the this pointer. Maybe we can even find a way to discuss these two issues in the RefPtr document without making it too confusing. Comments are welcome. But lets not bikeshed http://en.wikipedia.org/wiki/Parkinson's_Law_of_Triviality this! -- Darin ___ 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
Re: [webkit-dev] Lets use PassRefPtr for arguments less; lets use RefPtr for locals and data members more
On Jun 18, 2011, at 5:52 PM, Darin Adler wrote: 1: Recently, Alexey has encouraged me to use PassRefPtr less for function arguments. The PassRefPtr optimization pays off when the object in question is possibly being handed off from one PassRefPtr to another. For an argument, that can happen in two ways: 1) The argument can be the result of a function that returns a PassRefPtr. 2) The argument can be the result of calling release a local or data member that is a RefPtr. In both of those cases, we are transferring ownership. Mechanically speaking, PassRefPtr only pays off if we are actually getting that optimization. If we are passing a raw pointer, then using PassRefPtr for the function argument type doesn’t help much. It just puts a ref at the call site, a ref that otherwise would happen inside the function. It may even cause a bit of code bloat if there is more than one call site. Conceptually speaking, PassRefPtr only pays off if the context is a clear transfer of ownership. Passing an object that the recipient *might* later choose to take shared ownership of is not enough. Clients are always welcome to take shared ownership of something passed with a raw pointer. Because there are also costs to PassRefPtr, we should reserve PassRefPtr arguments for cases where the optimization will really pay off and for where the function definitely is taking ownership. Those functions do exist, but many current uses of PassRefPtr for arguments do not qualify. A few thoughts on this: - The benefit of PassRefPtr at any individual call site is probably too small to be measurable, but I believe taken together they all add up to somewhere in the ballpark of .2%-.5% on some benchmarks by avoiding refcount churn. - 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. - Longer term we can replace PassRefPtr with use of C++0x rvalue references. I wonder if it's possible to do this in a way where we use rvalue references on compilers that support them and classic PassRefPtr elsewhere. 2: Recently, I’ve noticed that many bugs simply would cease to exist if we used RefPtr more and raw pointer less for things like local variables and data members. The time it’s safe to use a raw pointer for a local variable or data member for a reference counted object pointer is when there is some guarantee that someone else is holding a reference. It can be difficult to have such a guarantee and those guarantees are fragile as code executes. Nowhere is that more clear than in loader-related code. Some are loathe to use RefPtr for data members because they are concerned about reference cycles. Generally speaking, we can eliminate the worry about reference cycles by making destruction include a “closing” process, which can null out all the references and break cycles. If we find it’s necessary we can also look into an efficient WeakPtr implementation. Some have been enthusiastic about this in the past. I have been a bit less so because I don’t know of an efficient implementation strategy. We could probably use RefPtr for at least some data members in the DOM, but it would be tricky to avoid introducing needless refcount churn. I guess this gets back to the point about PassRefPtr. Using RefPtr for locals is probably a good idea in some cases, but it can also lead to refcount churn, and refcount churn tends to make a program slower by death of a thousand cuts, so it's hard to find the source of the problem after the fact. I've wondered at times whether it might be a good idea to use a RefPtrT (note the reference) for cases where it's desirable to be guaranteed that someone holds a ref but you don't want to add an extra ref yourself. Conclusion: A specific example where an argument has type PassRefPtr, but that does not seem like the correct design, is the the node argument in the constructors of the various HTMLCollection classes. That's probably so, but I'd also guess the cost is smaller than maintaining clear understanding of why PassRefPtr should not be used here but should in other similar cases. i would still prefer to adhere to the simpler rule even though PassRefPtr doesn't add anything in this specific case. PassRefPtr also makes it harder to accidentally put an incoming pointer into a raw pointer data member or local variable, so it has a potential typechecking benefit even when it isn't helping your
Re: [webkit-dev] Lets use PassRefPtr for arguments less; lets use RefPtr for locals and data members more
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. - WBR, Alexey Proskuryakov ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
