Re: [webkit-dev] Changes to the WebKit2 development process
On Tuesday, January 08, 2013 02:57:53 PM Sam Weinig wrote: Hello webkit-dev, We are making some changes to the development process for WebKit2. These changes were announced to reviewers in advance, and I'd like to share them with you now. WebKit2 has a core set of functionality that is valuable to all ports, and then aspects that are only of limited/specialized interest. It is becoming increasingly difficult to improve and advance the core functionality while maintaining the more peripheral aspects. In addition, changes to the core often require significant expertise to evaluate, for instance to ensure that the security and responsiveness goals of WebKit2 are met. The changes are: 1) WebKit2 now has owners. Only owners should review WebKit2 patches. While we do not want to apply this concept across the whole WebKit project at this time, for WebKit2 it is appropriate. The list of owners is documented in the Owners file at the WebKit2 top level directory, and in committers.py. I think the fact that the regular WebKit review process stops at the boundary of WebKit2 should be documented in the WebKit Committers and Reviewer Policy. Simon ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Changes to the WebKit2 development process
On Wed, Jan 9, 2013 at 11:32 AM, Simon Hausmann simon.hausm...@digia.com wrote: On Tuesday, January 08, 2013 02:57:53 PM Sam Weinig wrote: Hello webkit-dev, We are making some changes to the development process for WebKit2. These changes were announced to reviewers in advance, and I'd like to share them with you now. WebKit2 has a core set of functionality that is valuable to all ports, and then aspects that are only of limited/specialized interest. It is becoming increasingly difficult to improve and advance the core functionality while maintaining the more peripheral aspects. In addition, changes to the core often require significant expertise to evaluate, for instance to ensure that the security and responsiveness goals of WebKit2 are met. The changes are: 1) WebKit2 now has owners. Only owners should review WebKit2 patches. While we do not want to apply this concept across the whole WebKit project at this time, for WebKit2 it is appropriate. The list of owners is documented in the Owners file at the WebKit2 top level directory, and in committers.py. I think the fact that the regular WebKit review process stops at the boundary of WebKit2 should be documented in the WebKit Committers and Reviewer Policy. Agree. And please clarify on the policy if we are talking about everything inside the WebKit2/ directory or if we have exceptions. It is not clear to me if port specific code is covered by this rule and should by reviewed by the owners. And what about code shared by Qt, GTK and EFL (i.e. Platform/CoreIPC/unix/) but not used by Mac? - Thiago ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Changes to the WebKit2 development process
Hi, 2013/1/8 Sam Weinig wei...@apple.com: Hello webkit-dev, We are making some changes to the development process for WebKit2. These changes were announced to reviewers in advance, and I'd like to share them with you now. WebKit2 has a core set of functionality that is valuable to all ports, and then aspects that are only of limited/specialized interest. It is becoming increasingly difficult to improve and advance the core functionality while maintaining the more peripheral aspects. In addition, changes to the core often require significant expertise to evaluate, for instance to ensure that the security and responsiveness goals of WebKit2 are met. Isn't that why we already differentiate between committers and reviewers? I mean, isn't like that throughout the entire project already? I thought _any_ patch to any part of WebKit required significant expertise to be evaluated. The changes are: 1) WebKit2 now has owners. Only owners should review WebKit2 patches. While we do not want to apply this concept across the whole WebKit project at this time, for WebKit2 it is appropriate. The list of owners is documented in the Owners file at the WebKit2 top level directory, and in committers.py. If I'm not mistaken, there are only people from the Mac port in the OWNERS file. Will there be some policy that other reviewers from other ports can become owners of WebKit2 as well, or will that be Apple-only always? 2) Ports must keep themselves building. Non Apple Mac ports, if broken by core functionality changes to WebKit2, are now responsible for fixing themselves. We have asked those who run the EWS bots to make sure that failing to build WebKit2 does not block the commit queue from committing. IMHO, doing this is breaking down an entire 'culture' of the WebKit workflow that we are all so proud of. 3) Over time, owners may remove peripheral functionality from the main WebKit2 directory, such as support for features that aren't broadly applicable. We will not do this immediately, and we will work with ports that are interested in such features to create appropriate, maintainable general-purpose mechanisms that can be used to implement them outside of core WebKit2 code. While we understand that this change will inconvenience some ports, we have decided that forward progress of WebKit2 is a more important concern, and we are moving forward with this change tonight. Well, at least from my side, I only got this email _after_ you had already moved forward with everything. I actually saw the patches landing way before it. Not cool! :) I thought the reviewers had all agreed about all these, but now after the first round of replies to this thread it is sad to see that not even among you guys there was a full settlement about this topic. Cheers, jesus - Sam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Changes to the WebKit2 development process
On Qua, 2013-01-09 at 12:04 +0200, Thiago Marcos P. Santos wrote: I think the fact that the regular WebKit review process stops at the boundary of WebKit2 should be documented in the WebKit Committers and Reviewer Policy. Agree. And please clarify on the policy if we are talking about everything inside the WebKit2/ directory or if we have exceptions. It is not clear to me if port specific code is covered by this rule and should by reviewed by the owners. And what about code shared by Qt, GTK and EFL (i.e. Platform/CoreIPC/unix/) but not used by Mac? Curious about this myself, I just reviewed a patch only affecting the GTK-specific parts of WebKit2, I believe that is OK? Should we ammend the Owners file to include information about port-specific directories and reviewers? Cheers, -- Gustavo Noronha Silva g...@gnome.org GNOME Project ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Changes to the WebKit2 development process
I've got a patch in flight that adds a feature flag. https://bugs.webkit.org/show_bug.cgi?id=106275 According to the instructions liked below I need to edit a WebKit2 file http://trac.webkit.org/wiki/AddingFeatures#ActivatingafeatureforAutotoolsbasedports Does that guideline change? Should I remove the WebKit2 change? On Tue, Jan 8, 2013 at 2:57 PM, Sam Weinig wei...@apple.com wrote: Hello webkit-dev, We are making some changes to the development process for WebKit2. These changes were announced to reviewers in advance, and I'd like to share them with you now. WebKit2 has a core set of functionality that is valuable to all ports, and then aspects that are only of limited/specialized interest. It is becoming increasingly difficult to improve and advance the core functionality while maintaining the more peripheral aspects. In addition, changes to the core often require significant expertise to evaluate, for instance to ensure that the security and responsiveness goals of WebKit2 are met. The changes are: 1) WebKit2 now has owners. Only owners should review WebKit2 patches. While we do not want to apply this concept across the whole WebKit project at this time, for WebKit2 it is appropriate. The list of owners is documented in the Owners file at the WebKit2 top level directory, and in committers.py. 2) Ports must keep themselves building. Non Apple Mac ports, if broken by core functionality changes to WebKit2, are now responsible for fixing themselves. We have asked those who run the EWS bots to make sure that failing to build WebKit2 does not block the commit queue from committing. 3) Over time, owners may remove peripheral functionality from the main WebKit2 directory, such as support for features that aren't broadly applicable. We will not do this immediately, and we will work with ports that are interested in such features to create appropriate, maintainable general-purpose mechanisms that can be used to implement them outside of core WebKit2 code. While we understand that this change will inconvenience some ports, we have decided that forward progress of WebKit2 is a more important concern, and we are moving forward with this change tonight. - Sam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Changes to the WebKit2 development process
Trivial changes like this do not need to be approved by an owner. -Sam On Jan 9, 2013, at 9:38 AM, Gregg Tavares g...@google.com wrote: I've got a patch in flight that adds a feature flag. https://bugs.webkit.org/show_bug.cgi?id=106275 According to the instructions liked below I need to edit a WebKit2 file http://trac.webkit.org/wiki/AddingFeatures#ActivatingafeatureforAutotoolsbasedports Does that guideline change? Should I remove the WebKit2 change? On Tue, Jan 8, 2013 at 2:57 PM, Sam Weinig wei...@apple.com wrote: Hello webkit-dev, We are making some changes to the development process for WebKit2. These changes were announced to reviewers in advance, and I'd like to share them with you now. WebKit2 has a core set of functionality that is valuable to all ports, and then aspects that are only of limited/specialized interest. It is becoming increasingly difficult to improve and advance the core functionality while maintaining the more peripheral aspects. In addition, changes to the core often require significant expertise to evaluate, for instance to ensure that the security and responsiveness goals of WebKit2 are met. The changes are: 1) WebKit2 now has owners. Only owners should review WebKit2 patches. While we do not want to apply this concept across the whole WebKit project at this time, for WebKit2 it is appropriate. The list of owners is documented in the Owners file at the WebKit2 top level directory, and in committers.py. 2) Ports must keep themselves building. Non Apple Mac ports, if broken by core functionality changes to WebKit2, are now responsible for fixing themselves. We have asked those who run the EWS bots to make sure that failing to build WebKit2 does not block the commit queue from committing. 3) Over time, owners may remove peripheral functionality from the main WebKit2 directory, such as support for features that aren't broadly applicable. We will not do this immediately, and we will work with ports that are interested in such features to create appropriate, maintainable general-purpose mechanisms that can be used to implement them outside of core WebKit2 code. While we understand that this change will inconvenience some ports, we have decided that forward progress of WebKit2 is a more important concern, and we are moving forward with this change tonight. - Sam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Changes to the WebKit2 development process
On Jan 9, 2013, at 8:10 AM, Gustavo Noronha Silva g...@gnome.org wrote: On Qua, 2013-01-09 at 12:04 +0200, Thiago Marcos P. Santos wrote: I think the fact that the regular WebKit review process stops at the boundary of WebKit2 should be documented in the WebKit Committers and Reviewer Policy. Agree. And please clarify on the policy if we are talking about everything inside the WebKit2/ directory or if we have exceptions. It is not clear to me if port specific code is covered by this rule and should by reviewed by the owners. And what about code shared by Qt, GTK and EFL (i.e. Platform/CoreIPC/unix/) but not used by Mac? Curious about this myself, I just reviewed a patch only affecting the GTK-specific parts of WebKit2, I believe that is OK? Should we ammend the Owners file to include information about port-specific directories and reviewers? Cheers, At this point, we ask that all completely non-trivial patches be reviewed by an owner, even if in port specific code. - Sam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Changes to the WebKit2 development process
Hi Sam. Some comments below. Cheers, --Antonio Curious about this myself, I just reviewed a patch only affecting the GTK-specific parts of WebKit2, I believe that is OK? Should we ammend the Owners file to include information about port-specific directories and reviewers? Cheers, At this point, we ask that all completely non-trivial patches be reviewed by an owner, even if in port specific code. First, I would like to say that I understand the frustration you guys might have faced by not being able to move Core WebKit2 development at the speed you guys think you could go due to other WebKit2 ports. That is indeed not the goal of the project, and likely the first time the project has seen it at this scale (correct if I am wrong, please). Further, although I do not fully support the direction pointed out as the solution to this problem, I have to agree that it might work. However, I am wondering if the new Core WK2 owners would really feel comfortable in reviewing Qt, Gtk and EFL specific WK2 patches - given that they are likely unfamiliar with the code. Does not it go against the primary *rule* of the WebKit reviewership process, where a reviewer is only allowed to R+ a patch he/she fully understands? If we had a concept of a super-review instead, like Firefox, where owners sometimes rubber stamp patches even when they do not have the know-how to reliably review it, given that the patch has got an r+ of someone else that actually does. Maybe your last email was that one that actually scared me. Looking forward for you reply, ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Int/FloatPoint and Int/FloatSize
Also, the word position is used to represent a tree-position in DOM in WebKit so please don't use that to represent a point in a screen or a layout coordinate system. OK, perhaps we should use Vector2d, as is used by the Chromium port ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Int/FloatPoint and Int/FloatSize
On Jan 9, 2013, at 3:47 PM, Steve Block stevebl...@chromium.org wrote: Also, the word position is used to represent a tree-position in DOM in WebKit so please don't use that to represent a point in a screen or a layout coordinate system. OK, perhaps we should use Vector2d, as is used by the Chromium port I don't like that name. I'm really not sure that this set of changes is going in the right direction. What's driving them; some abstract sense of purity, or reducing the chances of introducing real bugs that we've seen in the past? Simon ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Int/FloatPoint and Int/FloatSize
Removing the existing subtraction operator (xxxPoint minus xxxPoint returns xxxSize) might be a good place to start. I've uploaded a patch to https://bugs.webkit.org/show_bug.cgi?id=106408 which replaces these subtraction operators with ones that return xxxPoint, and which adds Int/FloatSize::fromCornerPoints(). The patch does just enough to make these changes, though in many cases, the switch from xxxSize to xxxPoint could be propagated further up and down the stack. However, the patch is probably big enough as it is, so this should probably be done separately, on a case-by-case basis. Note that the patch isn't ready for formal review, but I wanted to get some feedback before I pursue it further. Steve ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Int/FloatPoint and Int/FloatSize
On Wed, Jan 9, 2013 at 3:47 PM, Steve Block stevebl...@chromium.org wrote: Also, the word position is used to represent a tree-position in DOM in WebKit so please don't use that to represent a point in a screen or a layout coordinate system. OK, perhaps we should use Vector2d, as is used by the Chromium port My reference, if we’re doing this rename after addressing concerns expressed by Simon and others, is TwoDimensionalVector, R2Vector, or VectorInR2 because it’s a vector in a two-dimensional vector space, not a 2d of type vector. - R. Niwa ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Int/FloatPoint and Int/FloatSize
I'm really not sure that this set of changes is going in the right direction. What's driving them; some abstract sense of purity, or reducing the chances of introducing real bugs that we've seen in the past? Some of both. I was investigating a bug where WebCore is generating unexpected negative sizes. While looking into how this could happen, I noticed that in some cases, negative sizes result from size types being used to represent things that seem more like points. It seems like it would be good to clean this up this misuse of size types, and perhaps in the long term, we can avoid negative sizes altogether, which would help avoid these kinds of bug in the future. ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Int/FloatPoint and Int/FloatSize
On Jan 9, 2013, at 4:52 PM, Steve Block stevebl...@chromium.org wrote: I'm really not sure that this set of changes is going in the right direction. What's driving them; some abstract sense of purity, or reducing the chances of introducing real bugs that we've seen in the past? Some of both. I was investigating a bug where WebCore is generating unexpected negative sizes. While looking into how this could happen, I noticed that in some cases, negative sizes result from size types being used to represent things that seem more like points. It seems like it would be good to clean this up this misuse of size types, and perhaps in the long term, we can avoid negative sizes altogether, which would help avoid these kinds of bug in the future. It seems like a lot of churn for relatively little gain. I'd rather our time be focused on areas that actually benefit users of WebKit. Simon ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
[webkit-dev] Feature Announcement: Moving HTML Parser off the Main Thread
We're planning to move parts of the HTML Parser off of the main thread: https://bugs.webkit.org/show_bug.cgi?id=106127 This is driven by our testing showing that HTML parsing on mobile is be slow, and long (causing user-visible delays averaging 10 frames / 150ms). https://bug-106127-attachments.webkit.org/attachment.cgi?id=182002 Complete data can be found at [1]. Mozilla moved their parser onto a separate thread during their HTML5 parser re-write: https://developer.mozilla.org/en-US/docs/Mozilla/Gecko/HTML_parser_threading We plan to take a slightly simpler approach, moving only Tokenizing off of the main thread: https://docs.google.com/drawings/d/1hwYyvkT7HFLAtTX_7LQp2lxA6LkaEWkXONmjtGCQjK0/edit The left is our current design, the middle is a tokenizer-only design, and the right is more like mozilla's threaded-parser design. Profiling shows Tokenizing accounts for about 10x the number of samples as TreeBuilding. Including Antti's recent testing (.5% vs. 3%): https://bugs.webkit.org/show_bug.cgi?id=106127#c10 If after we do this we measure and find ourselves still spending a lot of main-thread time parsing, we'll move the TreeBuilder too. :) (This work is a nicely separable sub-set of larger work needed to move the TreeBuilder.) We welcome your thoughts and comments. 1. https://docs.google.com/spreadsheet/ccc?key=0AlC4tS7Ao1fIdGtJTWlSaUItQ1hYaDFDcWkzeVAxOGc#gid=0 (Epic thanks to Nat Duca for helping us collect that data.) ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Int/FloatPoint and Int/FloatSize
It seems like a lot of churn for relatively little gain. I'd rather our time be focused on areas that actually benefit users of WebKit. OK. I don't feel strongly enough about this to push the issue. ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Feature Announcement: Moving HTML Parser off the Main Thread
On Wed, Jan 9, 2013 at 6:00 PM, Eric Seidel e...@webkit.org wrote: We're planning to move parts of the HTML Parser off of the main thread: https://bugs.webkit.org/show_bug.cgi?id=106127 This is driven by our testing showing that HTML parsing on mobile is be slow, and long (causing user-visible delays averaging 10 frames / 150ms). https://bug-106127-attachments.webkit.org/attachment.cgi?id=182002 Complete data can be found at [1]. In case it's not clear from that link, the ParseHTML column is the total amount of time the web inspector attributes to HTML parsing when loading those URLs on a Nexus 7 using a top-of-tree build of Chromium's content_shell (similar to WebKitTestRunner). The HTML parser parses data a chunk at a time, which means the total time doesn't tell the whole story. The ParseHTML_max column shows the largest single block of time spent in the HTML parser, which is more of a measure of the main thread jank caused by the parser. Antti has pointed out that the inspector isn't the best source of data. He measured total time using instruments, and got numbers that are consistent (within a factor of 2) of the inspector measurements. (We were using different data sets, so we wouldn't expect perfect agreement even if we were measuring precisely the same thing.) Adam Mozilla moved their parser onto a separate thread during their HTML5 parser re-write: https://developer.mozilla.org/en-US/docs/Mozilla/Gecko/HTML_parser_threading We plan to take a slightly simpler approach, moving only Tokenizing off of the main thread: https://docs.google.com/drawings/d/1hwYyvkT7HFLAtTX_7LQp2lxA6LkaEWkXONmjtGCQjK0/edit The left is our current design, the middle is a tokenizer-only design, and the right is more like mozilla's threaded-parser design. Profiling shows Tokenizing accounts for about 10x the number of samples as TreeBuilding. Including Antti's recent testing (.5% vs. 3%): https://bugs.webkit.org/show_bug.cgi?id=106127#c10 If after we do this we measure and find ourselves still spending a lot of main-thread time parsing, we'll move the TreeBuilder too. :) (This work is a nicely separable sub-set of larger work needed to move the TreeBuilder.) We welcome your thoughts and comments. 1. https://docs.google.com/spreadsheet/ccc?key=0AlC4tS7Ao1fIdGtJTWlSaUItQ1hYaDFDcWkzeVAxOGc#gid=0 (Epic thanks to Nat Duca for helping us collect that data.) ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Feature Announcement: Moving HTML Parser off the Main Thread
How will we ensure thread safety? Even at just the tokenizing level don't we use AtomicString? AtromicString isn't threadsafe wrt StringImpl IIRC so this seems like it sould add a world of hurt. I realise it's been a long time since I've worked on this so it's completely possible that I'm not aware of the current behaviour. That aside I question what the benefit of this will be. All those cases where we've started parsing html are intrinsically tied to the web's general single thread of execution model, which implies that even if we do push parsing into a separate thread we'll just end up with the ui thread blocked on the parsing thread which doesn't seem hugely superior. What is the objective here? To improve performance, add parallelism, or reduce latency? --Oliver On Jan 9, 2013, at 6:10 PM, Adam Barth aba...@webkit.org wrote: On Wed, Jan 9, 2013 at 6:00 PM, Eric Seidel e...@webkit.org wrote: We're planning to move parts of the HTML Parser off of the main thread: https://bugs.webkit.org/show_bug.cgi?id=106127 This is driven by our testing showing that HTML parsing on mobile is be slow, and long (causing user-visible delays averaging 10 frames / 150ms). https://bug-106127-attachments.webkit.org/attachment.cgi?id=182002 Complete data can be found at [1]. In case it's not clear from that link, the ParseHTML column is the total amount of time the web inspector attributes to HTML parsing when loading those URLs on a Nexus 7 using a top-of-tree build of Chromium's content_shell (similar to WebKitTestRunner). The HTML parser parses data a chunk at a time, which means the total time doesn't tell the whole story. The ParseHTML_max column shows the largest single block of time spent in the HTML parser, which is more of a measure of the main thread jank caused by the parser. Antti has pointed out that the inspector isn't the best source of data. He measured total time using instruments, and got numbers that are consistent (within a factor of 2) of the inspector measurements. (We were using different data sets, so we wouldn't expect perfect agreement even if we were measuring precisely the same thing.) Adam Mozilla moved their parser onto a separate thread during their HTML5 parser re-write: https://developer.mozilla.org/en-US/docs/Mozilla/Gecko/HTML_parser_threading We plan to take a slightly simpler approach, moving only Tokenizing off of the main thread: https://docs.google.com/drawings/d/1hwYyvkT7HFLAtTX_7LQp2lxA6LkaEWkXONmjtGCQjK0/edit The left is our current design, the middle is a tokenizer-only design, and the right is more like mozilla's threaded-parser design. Profiling shows Tokenizing accounts for about 10x the number of samples as TreeBuilding. Including Antti's recent testing (.5% vs. 3%): https://bugs.webkit.org/show_bug.cgi?id=106127#c10 If after we do this we measure and find ourselves still spending a lot of main-thread time parsing, we'll move the TreeBuilder too. :) (This work is a nicely separable sub-set of larger work needed to move the TreeBuilder.) We welcome your thoughts and comments. 1. https://docs.google.com/spreadsheet/ccc?key=0AlC4tS7Ao1fIdGtJTWlSaUItQ1hYaDFDcWkzeVAxOGc#gid=0 (Epic thanks to Nat Duca for helping us collect that data.) ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Int/FloatPoint and Int/FloatSize
On Jan 9, 2013, at 5:40 PM, Simon Fraser simon.fra...@apple.com wrote: On Jan 9, 2013, at 4:52 PM, Steve Block stevebl...@chromium.org wrote: I'm really not sure that this set of changes is going in the right direction. What's driving them; some abstract sense of purity, or reducing the chances of introducing real bugs that we've seen in the past? Some of both. I was investigating a bug where WebCore is generating unexpected negative sizes. While looking into how this could happen, I noticed that in some cases, negative sizes result from size types being used to represent things that seem more like points. It seems like it would be good to clean this up this misuse of size types, and perhaps in the long term, we can avoid negative sizes altogether, which would help avoid these kinds of bug in the future. It seems like a lot of churn for relatively little gain. I'd rather our time be focused on areas that actually benefit users of WebKit. I agree with this concern. Another object for representing 2D vector data will cause more maintenance cost now and in the future. And I doubt that it gives a big enough benefit over defining when to use Point and Size today. Greetings, Dirk Simon ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Feature Announcement: Moving HTML Parser off the Main Thread
On Wed, Jan 9, 2013 at 6:38 PM, Oliver Hunt oli...@apple.com wrote: How will we ensure thread safety? Even at just the tokenizing level don't we use AtomicString? AtromicString isn't threadsafe wrt StringImpl IIRC so this seems like it sould add a world of hurt. AtomicString is already usable from other threads (http://trac.webkit.org/changeset/38094), but are correct this is the core concern! PickledToken (or whatever it's called) will have to be written very carefully in order to minimize/eliminate copies, while still guaranteeing thread safety. The correct design and handling of PickledToken is the entire question of this whole endeavor. I realise it's been a long time since I've worked on this so it's completely possible that I'm not aware of the current behavior. That aside I question what the benefit of this will be. All those cases where we've started parsing html are intrinsically tied to the web's general single thread of execution model, which implies that even if we do push parsing into a separate thread we'll just end up with the ui thread blocked on the parsing thread which doesn't seem hugely superior. What is the objective here? To improve performance, add parallelism, or reduce latency? The core goal is to reduce latency -- to free up the main thread for JavaScript and UI interaction -- which as you correctly note, cannot be moved off of the main thread due to the single thread of execution model of the web. One could view the pre-load scanner as a lay-man's attempt at this type of tokenize asynchronously approach. This model gets preload scanning for free, as well as can easily answer wkb.ug/90751 request to speculative tokenizing of the entire document. (We just have to save markers before every script token, as if the script uses document.write, any tokens after /script become invalid.) I should also note that not all HTML parsing can be moved off of the main thread. innerHTML for example, would still be done entirely on the main thread. I would imagine that when we were to land this on trunk it would be behind a feature flag and ports could opt-in to the threaded-parsing path, as we must maintain the main-thread parsing ability for innerHTML anyway. --Oliver On Jan 9, 2013, at 6:10 PM, Adam Barth aba...@webkit.org wrote: On Wed, Jan 9, 2013 at 6:00 PM, Eric Seidel e...@webkit.org wrote: We're planning to move parts of the HTML Parser off of the main thread: https://bugs.webkit.org/show_bug.cgi?id=106127 This is driven by our testing showing that HTML parsing on mobile is be slow, and long (causing user-visible delays averaging 10 frames / 150ms). https://bug-106127-attachments.webkit.org/attachment.cgi?id=182002 Complete data can be found at [1]. In case it's not clear from that link, the ParseHTML column is the total amount of time the web inspector attributes to HTML parsing when loading those URLs on a Nexus 7 using a top-of-tree build of Chromium's content_shell (similar to WebKitTestRunner). The HTML parser parses data a chunk at a time, which means the total time doesn't tell the whole story. The ParseHTML_max column shows the largest single block of time spent in the HTML parser, which is more of a measure of the main thread jank caused by the parser. Antti has pointed out that the inspector isn't the best source of data. He measured total time using instruments, and got numbers that are consistent (within a factor of 2) of the inspector measurements. (We were using different data sets, so we wouldn't expect perfect agreement even if we were measuring precisely the same thing.) Adam Mozilla moved their parser onto a separate thread during their HTML5 parser re-write: https://developer.mozilla.org/en-US/docs/Mozilla/Gecko/HTML_parser_threading We plan to take a slightly simpler approach, moving only Tokenizing off of the main thread: https://docs.google.com/drawings/d/1hwYyvkT7HFLAtTX_7LQp2lxA6LkaEWkXONmjtGCQjK0/edit The left is our current design, the middle is a tokenizer-only design, and the right is more like mozilla's threaded-parser design. Profiling shows Tokenizing accounts for about 10x the number of samples as TreeBuilding. Including Antti's recent testing (.5% vs. 3%): https://bugs.webkit.org/show_bug.cgi?id=106127#c10 If after we do this we measure and find ourselves still spending a lot of main-thread time parsing, we'll move the TreeBuilder too. :) (This work is a nicely separable sub-set of larger work needed to move the TreeBuilder.) We welcome your thoughts and comments. 1. https://docs.google.com/spreadsheet/ccc?key=0AlC4tS7Ao1fIdGtJTWlSaUItQ1hYaDFDcWkzeVAxOGc#gid=0 (Epic thanks to Nat Duca for helping us collect that data.) ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list
Re: [webkit-dev] Feature Announcement: Moving HTML Parser off the Main Thread
On Wed, Jan 9, 2013 at 7:07 PM, Eric Seidel e...@webkit.org wrote: On Wed, Jan 9, 2013 at 6:38 PM, Oliver Hunt oli...@apple.com wrote: How will we ensure thread safety? Even at just the tokenizing level don't we use AtomicString? AtromicString isn't threadsafe wrt StringImpl IIRC so this seems like it sould add a world of hurt. AtomicString is already usable from other threads (http://trac.webkit.org/changeset/38094), but are correct this is the core concern! PickledToken (or whatever it's called) will have to be written very carefully in order to minimize/eliminate copies, while still guaranteeing thread safety. The correct design and handling of PickledToken is the entire question of this whole endeavor. That is probably what you meant, but just in case... AtomicString can be used from different threads, but is not thread safe. You must make an isolatedCopy() for message passing if you keep a reference to the String in your thread. Not the end of the world, but something to be aware of :) Cheers, Benjamin ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Feature Announcement: Moving HTML Parser off the Main Thread
On Wed, Jan 9, 2013 at 7:35 PM, Benjamin Poulain benja...@webkit.org wrote: On Wed, Jan 9, 2013 at 7:07 PM, Eric Seidel e...@webkit.org wrote: On Wed, Jan 9, 2013 at 6:38 PM, Oliver Hunt oli...@apple.com wrote: How will we ensure thread safety? Even at just the tokenizing level don't we use AtomicString? AtromicString isn't threadsafe wrt StringImpl IIRC so this seems like it sould add a world of hurt. AtomicString is already usable from other threads (http://trac.webkit.org/changeset/38094), but are correct this is the core concern! PickledToken (or whatever it's called) will have to be written very carefully in order to minimize/eliminate copies, while still guaranteeing thread safety. The correct design and handling of PickledToken is the entire question of this whole endeavor. That is probably what you meant, but just in case... AtomicString can be used from different threads, but is not thread safe. You must make an isolatedCopy() for message passing if you keep a reference to the String in your thread. Not the end of the world, but something to be aware of :) Yeah, we're aware of this issue. We'll probably end up doing something slightly customized for this use case. For example, many of the AtomicStrings used in parsing are tag and attribute names that are known at compile time (e.g., div, href). When moving these strings back to the main thread, we need only the hash of the string and not the underlying characters in the string (because we know statically that the hash will exist in the main thread's atomic string table). It's tempting to optimize these things prematurely. We'll likely start with a simple approach that makes copies and then optimize away the copies over the development of the feature as indicated by profiles. Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Feature Announcement: Moving HTML Parser off the Main Thread
I think your biggest challenge will be ensuring that the latency of shoving things to another core and then shoving them back will be smaller than the latency of processing those same things on the main thread. For small documents, I expect concurrent tokenization to be a pure regression because the latency of waking up another thread to do just a small bit of work, plus the added cost of whatever synchronization operations will be needed to ensure safety, will involve more total work than just tokenizing locally. We certainly see this in the JSC parallel GC, and in line with traditional parallel GC design, we ensure that parallel threads only kick in when the main thread is unable to keep up with the work that it has created for itself. Do you have a vision for how to implement a similar self-throttling, where tokenizing continues on the main thread so long as it is cheap to do so? -Filip On Jan 9, 2013, at 6:00 PM, Eric Seidel e...@webkit.org wrote: We're planning to move parts of the HTML Parser off of the main thread: https://bugs.webkit.org/show_bug.cgi?id=106127 This is driven by our testing showing that HTML parsing on mobile is be slow, and long (causing user-visible delays averaging 10 frames / 150ms). https://bug-106127-attachments.webkit.org/attachment.cgi?id=182002 Complete data can be found at [1]. Mozilla moved their parser onto a separate thread during their HTML5 parser re-write: https://developer.mozilla.org/en-US/docs/Mozilla/Gecko/HTML_parser_threading We plan to take a slightly simpler approach, moving only Tokenizing off of the main thread: https://docs.google.com/drawings/d/1hwYyvkT7HFLAtTX_7LQp2lxA6LkaEWkXONmjtGCQjK0/edit The left is our current design, the middle is a tokenizer-only design, and the right is more like mozilla's threaded-parser design. Profiling shows Tokenizing accounts for about 10x the number of samples as TreeBuilding. Including Antti's recent testing (.5% vs. 3%): https://bugs.webkit.org/show_bug.cgi?id=106127#c10 If after we do this we measure and find ourselves still spending a lot of main-thread time parsing, we'll move the TreeBuilder too. :) (This work is a nicely separable sub-set of larger work needed to move the TreeBuilder.) We welcome your thoughts and comments. 1. https://docs.google.com/spreadsheet/ccc?key=0AlC4tS7Ao1fIdGtJTWlSaUItQ1hYaDFDcWkzeVAxOGc#gid=0 (Epic thanks to Nat Duca for helping us collect that data.) ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Feature Announcement: Moving HTML Parser off the Main Thread
On Wed, 9 Jan 2013, Eric Seidel wrote: The core goal is to reduce latency -- to free up the main thread for JavaScript and UI interaction -- which as you correctly note, cannot be moved off of the main thread due to the single thread of execution model of the web. Parsing and (maybe to a lesser extent) compiling JS can be moved off the main thread, though, right? That's probably worth examining too, if it hasn't already been done. -- Ian Hickson U+1047E)\._.,--,'``.fL http://ln.hixie.ch/ U+263A/, _.. \ _\ ;`._ ,. Things that are impossible just take longer. `._.-(,_..'--(,_..'`-.;.' ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Feature Announcement: Moving HTML Parser off the Main Thread
On Jan 9, 2013, at 10:04 PM, Ian Hickson i...@hixie.ch wrote: On Wed, 9 Jan 2013, Eric Seidel wrote: The core goal is to reduce latency -- to free up the main thread for JavaScript and UI interaction -- which as you correctly note, cannot be moved off of the main thread due to the single thread of execution model of the web. Parsing and (maybe to a lesser extent) compiling JS can be moved off the main thread, though, right? That's probably worth examining too, if it hasn't already been done. 100% agree. However, the same problem I brought up about tokenization applies here: a lot of JS functions are super cheap to parse and compile already, and the latency of doing so on the main thread is likely to be lower than the latency of chatting with another core. I suspect this could be alleviated by (1) aggressively pipelining the work, where during page load or during heavy JS use the compilation thread always has a non-empty queue of work to do; this will mean that the latency of communication is paid only when the first compilation occurs, and (2) allowing the main thread to steal work from the compilation queue. I'm not sure how to make (2) work well. For parsing it's actually harder since we rely heavily on the lazy parsing optimization: code is only parsed once we need it *right now* to run a function. For compilation, it's somewhat easier: the most expensive compilation step is the third-tier optimizing JIT; we can delay this as long as we want, though the longer we dela y it, the longer we spend running slower code. Hence, to make parsing concurrent, the main problem is figuring out how to do predictive parsing: have a concurrent thread start parsing something just before we need it. Without predictive parsing, making it concurrent would be a guaranteed loss since the main thread would just be stuck waiting for the thread to finish. To make optimized compiles concurrent without a regression, the main problem is ensuring that in those cases where we believe that the time taken to compile the function will be smaller than the time taken to awake the concurrent thread, we will instead just compile it on the main thread right away. Though, if we could predict that a function was going to get hot in the future, we could speculatively tell a concurrent thread to compile it fully knowing that it won't wake up and do so until exactly when we would have otherwise invoked the compiler on the main thread (that is, it'll wake up and start compiling it once the main thread has executed the function enough times to get good profiling data). Anyway, you're absolutely right that this is an area that should be explored. -F -- Ian Hickson U+1047E)\._.,--,'``.fL http://ln.hixie.ch/ U+263A/, _.. \ _\ ;`._ ,. Things that are impossible just take longer. `._.-(,_..'--(,_..'`-.;.' ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
