Bug 38906

Created attachment 55703 [details] [PATCH] Suggested solution

Comment on attachment 55703 [details] [PATCH] Suggested solution WebCore/css/CSSParser.cpp:5482 + void CSSParser::markStart() I think you should pick a more specific name for these methods, something like markCurrentRuleStart

Are these visible to web pages, or only to the web inspector? If we change the web platform we need to evaluate the impact of adding this, but the bar is much lower if it’s just web inspector.

What's the impact on memory use?

Eight bytes added to every style rule? Really? I think we need to discuss this a bit. Also, Yury, please don't review core CSS changes like this.

Comment on attachment 55703 [details] [PATCH] Suggested solution Clearing review flag.

I did not have a chance to look at this change yet, but my plan was to make a sanity check and then ask Dave to take a look at it in case it was Ok. Please consider Yury's change an unfortunate accident. The idea behind the change is that we not only store line numbers for rules, but store their offsets as well. We need it for a new web inspector feature. Here is the user story for it: - User modifies CSS using elements panel sidebar in inspector - We show a 'Local changes' tab next to the 'Content' tab in the resources panel of inspector - This tab contains all the changes user has applied merged into the original source - User has ability to copy new content to the clipboard and apply it to real source Implementing a parallel full-blown CSS parser did not seem a good idea, storing token positions for property tokens in the original one also seemed like a bad idea (memory). So we decided to use a hybrid approach where we rely on original CSS parser to do the rules markup, then we parse individual properties on the inspector side. Alexander has the proof of concept change with this running already, so he started splitting it into patches. Talking about the ways to test this one. When I was adding rule's line numbers I've done following to make sure we did not regress performance / memory: - I was running chromium page cycler and was making sure there was no memory regressions (or at least they were not visible given the measurement accuracy) - I had a giant (4Mb) CSS file that I created via concatenating all the page cycler CSS files (real world ones) and was measuring the regression in speed. Latest version of the code was not regressing at all (fraction of a percent I guess). So I think we should do this again with this change. We'll be more accurate on the heap numbers and present them here before going forward. If we see it regressing, we could hide the changes behind a constructor parameter flag and manually re-parse with this flag enabled from within inspector backend should we need it.

(In reply to comment #7) > If we see it regressing, we could hide the changes behind a constructor parameter flag and manually re-parse with this flag enabled from within inspector backend should we need it. I think memory is the main concern, so this approch would not help with that.

(In reply to comment #8) > (In reply to comment #7) > > If we see it regressing, we could hide the changes behind a constructor parameter flag and manually re-parse with this flag enabled from within inspector backend should we need it. > > I think memory is the main concern, so this approch would not help with that. We could use external storage for that, passing reference in the constructor in place of the flag. It'll be tricky to make parsing conditional, but we can at least take a look at it.

Comment on attachment 55703 [details] [PATCH] Suggested solution WebCore/css/tokenizer.flex:125 + "{" {markStart(); yyTok = *yytext; return yyTok;} You don't need markStart since we already have a good way of capturing the '{' location (see updateLastSelectorLine, it is being called while on '{', so just take yytext there). WebCore/css/tokenizer.flex:126 + "}" {markEnd(); yyTok = *yytext; return yyTok;} Two options here: - You could get rid of this tokenizer change as well if you consider the end of your rule to be an offset of the token following the '}' - Leave this code here and get the mark in the ruleset action (where you currently call reset). WebCore/css/CSSStyleRule.cpp:32 + , m_sourceLine(sourceLine) I wonder if we could move to 16 bit for the line number + use 32 bit per offset start and 16 bit per rule length. I don't think we handle rules with more than 65K anyways. This gives a 32 bit regression per rule wrt present state. On my test with 4Mb CSS taken from all page cycle test pages, it would consume additional 160K. I wonder what is it comparing to the present struct size. I know you are measuring it right now, would be interesting to know what is the source code / runtime representation ratio for WebKit's CSS.

Experiment: time+space-wise comparison of current vs. [rule body start/end offset]-tracking CSSStyleRule implementation. Changes for offset tracking: - CSSStyleRule: * m_sourceLine (int -> unsigned short) * Added two fields (unsigned int and unsigned short for rule start offset + rule length) - CSSParser: * Added tracking of opening and closing braces for style rules (see the patch) Input: - Test stylesheet: all Chromium pagecycler CSS files concatenated (2746 kB, 3235 rules) Results: - Parse time (5 samples): * HEAD: Average = 45.677ms, StDev(sigma) = 0.305ms * With positions tracked: Average = 45.947ms, StDev(sigma) = 0.325ms - Memory implications: Space measured: SUM(sizeof(CSSStyleRule) + sizeof(CSSMutableStyleDeclaration) + SelectorLength) for each rule. * HEAD: Size = 456520 bytes (Windows, Debug), 353448 bytes (Mac, Release) * With positions tracked: Size = 482288 bytes (Windows, Debug), 379216 bytes (Mac, Release) Delta = 25768 bytes (5.6% for Windows, 7.3% for Mac) Conclusions: - Time delta (0.27ms) is within one sigma, which implies that the parse time is not affected. - Space delta is 8 bytes per CSSStyleRule (rather than the expected 4 bytes), presumably due to the data alignment (the m_sourceLine type change does not decrease CSSStyleRule size, leaving it at 120 bytes (Win)). Any comments from the pros' side?

Created attachment 56388 [details] [PATCH] External storage approach This patch implements an alternative approach: rule body start/end offsets are extracted into an external Vector (inside InspectorCSSStore) only when the Web Inspector side has requested to do so.

Attachment 56388 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebCore/css/CSSParser.h:210: More than one command on the same line [whitespace/newline] [4] WebCore/inspector/InspectorCSSStore.cpp:34: Alphabetical sorting problem. [build/include_order] [4] WebCore/inspector/InspectorCSSStore.cpp:48: Tab found; better to use spaces [whitespace/tab] [1] WebCore/css/CSSParser.cpp:1565: One space before end of line comments [whitespace/comments] [5] WebCore/css/CSSParser.cpp:4379: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] WebCore/css/CSSParser.cpp:4406: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] Total errors found: 6 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 56388 [details] [PATCH] External storage approach I suggest you make all the whitespace changes in an initial patch with no other source changes.

Created attachment 56484 [details] [PATCH] External storage: merged with a whitespace cleanup (39351), style fixed

Created attachment 56490 [details] [PATCH] Added missing maps to reset()

Created attachment 56598 [details] [PATCH] Minor cleanup

Reviewers, any input on this one?

Comment on attachment 56598 [details] [PATCH] Minor cleanup r=me

Created attachment 57009 [details] [PATCH] InspectorCSSStore: struct -> class

Comment on attachment 57009 [details] [PATCH] InspectorCSSStore: struct -> class Provided Dave's Ok with previous version, I am r+ing this. Please see notes and fix things prior to landing though. Thanks! ======= Now that the CSSStore is getting a solid API, it looks more like InspectorCSSAgent. We should rename it in one of subsequent changes. WebCore/inspector/InspectorCSSStore.cpp:83 + while (i < originalRuleListLength) { Please consider extracting this into a method. WebCore/inspector/InspectorCSSStore.cpp:95 + for (InspectorController::ResourcesMap::const_iterator resIt = m_inspectorController->resources().begin(); resIt != m_inspectorController->resources().end(); ++resIt) { I would introduce public InspectorController::resourceForURL(const String&) and use it here. One thing that bothers me is whether we can rely upon url matching here. CSS is often static though, so we should be Ok for now. WebCore/inspector/InspectorCSSStore.cpp:100 + p.parseSheet(newStyleSheet.get(), resIt->second->sourceString(), offsetVector); This might be blank in some cases. Will your code handle it well? WebCore/inspector/InspectorDOMAgent.cpp:1082 + CSSStyleSheet* inspectorStyleSheet = cssStore()->inspectorStyleSheet(); This whole logic can be incapsulated given that you pass node into the inspectorStyleSheet getter, right? Also note, that for different iframes we will use different inspector stylesheets, so you can't store single reference in CSS store. Please either fix here or file a bug.

Created attachment 57031 [details] [PATCH] pfeldman's comments addressed, inspectorStyleSheet bug fixed

Comment on attachment 57031 [details] [PATCH] pfeldman's comments addressed, inspectorStyleSheet bug fixed WebCore/css/CSSGrammar.y:893 + p->markEnd(); Nit: I would call markEnd and reset Marks from within createStyleRule just to minimize the coupling here (just as you do call markStart from within updateLastSelector*). WebCore/css/CSSParser.cpp:5500 + void CSSParser::markStart() nit: markRuleBodyStart? WebCore/css/CSSParser.cpp:5507 + void CSSParser::markEnd() markRuleBodyEnd WebCore/css/CSSParser.h:246 + void resetMarks() { m_startOffset = m_endOffset = 0; } resetRuleBodyMarks ? WebCore/css/CSSParser.h:241 + unsigned m_startOffset; m_ruleBodyStartOffset WebCore/css/CSSParser.h:242 + unsigned m_endOffset; m_ruleBodyEndOffset Please land it with comments addressed. There is no need to ask for another review. Reviewing same changes over and over after you've got r+ is painful. It is better to land as is and get a follow-up change reviewed in these cases.

Committing to http://svn.webkit.org/repository/webkit/trunk ... M LayoutTests/ChangeLog A LayoutTests/inspector/resources/styles-source-offsets.css A LayoutTests/inspector/styles-source-offsets-expected.txt A LayoutTests/inspector/styles-source-offsets.html M WebCore/ChangeLog M WebCore/css/CSSGrammar.y M WebCore/css/CSSParser.cpp M WebCore/css/CSSParser.h M WebCore/inspector/InspectorCSSStore.cpp M WebCore/inspector/InspectorCSSStore.h M WebCore/inspector/InspectorController.cpp M WebCore/inspector/InspectorController.h M WebCore/inspector/InspectorDOMAgent.cpp M WebCore/inspector/InspectorDOMAgent.h M WebCore/inspector/front-end/DOMAgent.js Committed r60227