Bug 94534

Created attachment 159531 [details] Patch

Comment on attachment 159531 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159531&action=review I buy the change, but fix the changelog. > Source/WebCore/ChangeLog:8 > + Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!). You should write something here.

(In reply to comment #2) > (From update of attachment 159531 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=159531&action=review > > I buy the change, but fix the changelog. > > > Source/WebCore/ChangeLog:8 > > + Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!). > > You should write something here. Whoops, I have a bad habit of forgetting to press save. :) Thanks. Fixing now.

Created attachment 159536 [details] Patch Trying again

Comment on attachment 159536 [details] Patch On second thought, I don't buy that this is right. This will give a pathological overestimation of the heap size. Why not have the GC save the result of its maxSize() computation in a field in Heap or JSGlobalData or something and then fetch that? That will be more accurate.

Created attachment 159540 [details] Patch Somehow the MemoryInfo.idl change didn't apply. Adding it back

(In reply to comment #5) > (From update of attachment 159536 [details]) > On second thought, I don't buy that this is right. This will give a pathological overestimation of the heap size. Why not have the GC save the result of its maxSize() computation in a field in Heap or JSGlobalData or something and then fetch that? That will be more accurate. That's totalJSHeapSize unless I misunderstand you. readonly attribute unsigned long totalJSHeapSize; readonly attribute unsigned long usedJSHeapSize; readonly attribute unsigned long jsHeapSizeLimit;

Comment on attachment 159540 [details] Patch See previous comment. This will be more correct if you save the result of computing maxSize() during GC.

(In reply to comment #7) > (In reply to comment #5) > > (From update of attachment 159536 [details] [details]) > > On second thought, I don't buy that this is right. This will give a pathological overestimation of the heap size. Why not have the GC save the result of its maxSize() computation in a field in Heap or JSGlobalData or something and then fetch that? That will be more accurate. > > That's totalJSHeapSize unless I misunderstand you. > > readonly attribute unsigned long totalJSHeapSize; > readonly attribute unsigned long usedJSHeapSize; > readonly attribute unsigned long jsHeapSizeLimit; Oh, sorry, our wires crossed. Let me look more carefully!

Comment on attachment 159540 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159540&action=review > Source/WebCore/bindings/js/ScriptGCEvent.cpp:51 > JSGlobalData* globalData = JSDOMWindow::commonJSGlobalData(); > info.totalJSHeapSize = globalData->heap.capacity(); > info.usedJSHeapSize = globalData->heap.size(); > - info.jsHeapSizeLimit = 0; > + info.jsHeapSizeLimit = globalData->heap.maxSize(); Two issues: 1) When is this called? My understanding is that this can be called at any time. 2) Even if this was only called during GC, you're doing a second walk over all blocks in the heap. That's what m_objectSpace.size() does. It would be *far better* if you had the GC save its result of computing maxSize() in a field and then grabbed that here. One less walk over the heap and it's guaranteed to be correct no matter when it's called.

(In reply to comment #9) > Oh, sorry, our wires crossed. Let me look more carefully! Twice actually :) No problem. My only concern was whether I should factor in the 2x multiplier on the limit or not, but I think it's more useful as it is and the expanded limit can be more of a safety net. Content that uses this value will hopefully be better behaved as a result. (In reply to comment #10) > (From update of attachment 159540 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=159540&action=review > > > Source/WebCore/bindings/js/ScriptGCEvent.cpp:51 > > JSGlobalData* globalData = JSDOMWindow::commonJSGlobalData(); > > info.totalJSHeapSize = globalData->heap.capacity(); > > info.usedJSHeapSize = globalData->heap.size(); > > - info.jsHeapSizeLimit = 0; > > + info.jsHeapSizeLimit = globalData->heap.maxSize(); > > Two issues: > > 1) When is this called? My understanding is that this can be called at any time. > > 2) Even if this was only called during GC, you're doing a second walk over all blocks in the heap. That's what m_objectSpace.size() does. It would be *far better* if you had the GC save its result of computing maxSize() in a field and then grabbed that here. > > One less walk over the heap and it's guaranteed to be correct no matter when it's called. I could do that. It can be called at any time. I guess then I'd have to call it at Heap creation time to initialize it too.

(In reply to comment #10) > 2) Even if this was only called during GC, you're doing a second walk over all blocks in the heap. That's what m_objectSpace.size() does. It would be *far better* if you had the GC save its result of computing maxSize() in a field and then grabbed that here. > > One less walk over the heap and it's guaranteed to be correct no matter when it's called. Actually how do you want to handle this? m_objectSpace.size() is only implicitly called by the new function. The real problem seems to be ::size() which was already there. I'm not sure it really matters about the performance for this specific call that much since it should probably only be called once at startup by a web app anyway. That said, fewer cycles = better.

(In reply to comment #12) > (In reply to comment #10) > > 2) Even if this was only called during GC, you're doing a second walk over all blocks in the heap. That's what m_objectSpace.size() does. It would be *far better* if you had the GC save its result of computing maxSize() in a field and then grabbed that here. > > > > One less walk over the heap and it's guaranteed to be correct no matter when it's called. > > Actually how do you want to handle this? m_objectSpace.size() is only implicitly called by the new function. The real problem seems to be ::size() which was already there. > > I'm not sure it really matters about the performance for this specific call that much since it should probably only be called once at startup by a web app anyway. That said, fewer cycles = better. Ping... don't want to start writing patches until I understand what you're after. I'm also very hesitant to change the size() code given that it looks like it could cause a lot of subtle breakage if not done just right.

Need a response on this. I don't see how reworking the class is related to this change. It just moves one piece of code into a more accessible area so it can be called again. if it's a problem, I think it's already a problem.

Comment on attachment 159540 [details] Patch Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.