Bug 73349

Created attachment 117039 [details] Patch

Comment on attachment 117039 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=117039&action=review r- for the unneeded size increase for each cell which is a definite NO. > Source/WebCore/rendering/RenderTable.h:206 > + } Nit: Would be better on a single line. > Source/WebCore/rendering/RenderTableCell.cpp:901 > + // The parent table is being laid out, so update our collapsed border cache. Looks like you could add this ASSERT here: ASSERT(!needsLayout()); > Source/WebCore/rendering/RenderTableCell.cpp:939 > + } It would be better to share this code with the previous function. That would provide a single point of failure + likely enhance our checks (like being able to add more ASSERTs). > Source/WebCore/rendering/RenderTableCell.h:119 > + void collectBorderValues(RenderTable::CollapsedBorderValues&); I would prefer to keep the const here and match logical constness definition. You will need to add the mutable property on your collapsed*Borders. > Source/WebCore/rendering/RenderTableCell.h:187 > + CollapsedBorderValue m_collapsedBottomBorder; No! You are adding 4 bytes to each table cell. Tables are already using a lot of memory and there is no way we are going to add more to it. Border collapse are not likely to be the common case (it's not the initial value for border-collapse) and you are increasing *every* table regardless of that. I would better see the cached borders attached to the section / table itself or in a rare field that gets allocated only if the table needs it (hack the styleDidChange / styleWillChange to properly allocate / deallocate the object).

Created attachment 117474 [details] Patch

Comment on attachment 117474 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=117474&action=review > Source/WebCore/rendering/RenderTable.h:203 > + bool invalidCollapsedBorders() The name of a boolean member function should finish the sentence “table <xxx>” so the name would be something like “has invalid collapsed borders” or “needs to recompute collapsed borders”. bool hasInvalidCollapsedBorders() const or bool needsToRecomputeCollapsedBorders() const > Source/WebCore/rendering/RenderTableSection.cpp:1316 > + m_cellCollapsedBorders.set(cell, newCellInfo); There’s a storage leak here if the map already has a stale CellCollapsedBorders for this cell.

Comment on attachment 117474 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=117474&action=review More comments. How do you enforce that no one actually call collapsed*Border and end up missing the cached values? We are getting closer but there are still a lot of rough edges (it is a difficult change though). >> Source/WebCore/rendering/RenderTable.h:203 >> + bool invalidCollapsedBorders() > > The name of a boolean member function should finish the sentence “table <xxx>” so the name would be something like “has invalid collapsed borders” or “needs to recompute collapsed borders”. > > bool hasInvalidCollapsedBorders() const > > or > > bool needsToRecomputeCollapsedBorders() const Also this should be on one line and const. I will let you pick the right name. > Source/WebCore/rendering/RenderTableCell.cpp:900 > + // The parent table is being laid out, so update our collapsed border cache. This looks bogus. We lazily update our borders during painting. Also you really don't want to ASSERT you are in the middle of a layout as some fields may be dirty! > Source/WebCore/rendering/RenderTableCell.cpp:905 > + addBorderStyle(borderValues, borders->right()); This changes looks wrong: you used to return the start border not the top border. Depending on your writing mode, those will be different. > Source/WebCore/rendering/RenderTableCell.cpp:933 > + collapsedBorders = section()->cellCollapsedBorders(this); These lines need to be wrapped in a new method to hide the caching mechanism (the caller should not care about it). > Source/WebCore/rendering/RenderTableCell.cpp:938 > + CollapsedBorderValue collapsedRightBorder = collapsedBorders->right(); Kudos on the better naming. > Source/WebCore/rendering/RenderTableSection.cpp:76 > + deleteAllCellCollapsedBorders(); This would be unneeded if ~HashMap hold instances. > Source/WebCore/rendering/RenderTableSection.h:133 > + CellCollapsedBorders* cellCollapsedBorders(const RenderTableCell*); I prefer collapsedBorderForCell which makes it more obvious what it does. > Source/WebCore/rendering/RenderTableSection.h:193 > + HashMap<const RenderTableCell*, CellCollapsedBorders*> m_cellCollapsedBorders; It would be better to store the instance of CellCollapsedBorders and not a pointer to it here. It would make the code easier to read and more bullet-proof (you don't expect your cache to return a NULL pointer). > Source/WebCore/rendering/style/CollapsedBorderValue.h:83 > + CollapsedBorderValue m_right; I would rather see us store the start, end, before, after values as those are the "primary" values. top, bottom, left, right are dependent on your writing mode and this looks like an unneeded dependency here.

This bug is invalid now due to revisions at 71244.

(In reply to comment #6) > This bug is invalid now due to revisions at 71244. I don't think it is. This bug may not be blocking bug 71244 anymore but we are still not caching the borders / calling compareBorders and it is hurting us (see bug 38810 or bug 66184 as 2 examples of that).

Created attachment 120555 [details] Patch

Comment on attachment 120555 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=120555&action=review I like the new approach a bit better as it guarantees we won't miss the cache. The logic could be made a lot more readable though. > Source/WebCore/rendering/RenderTable.cpp:299 > + // and cached when recomputing the table's width. s/width/logical width/. Also the comment is actually lacking half of the explanation: computing the logical width assures that we do resolve the collapsed borders in the inline base direction (before/after cells) not in the block flow direction. I would really love to have a comment explaining why *all* borders will be computed during layout. This would make sense as unfortunately there is no safe-guard against forgetting to compute a cell's border in your new caching code. I am not sure how difficult this would be to add so I am leaving this for you to see. > Source/WebCore/rendering/RenderTable.h:203 > + bool hasInvalidCollapsedBorders() { return !m_collapsedBordersValid; } should be |const| > Source/WebCore/rendering/RenderTableCell.cpp:422 > - return result; > + return cellCollapsedBorders->start(result); > } else if (isStartColumn) { > // (3) Our row's start border. > result = chooseBorder(result, CollapsedBorderValue(parent()->style()->borderStart(), includeColor ? parent()->style()->visitedDependentColor(startColorProperty) : Color(), BROW)); > if (!result.exists()) > - return result; > - > + return cellCollapsedBorders->start(result); > + > // (4) Our row group's start border. > result = chooseBorder(result, CollapsedBorderValue(section()->style()->borderStart(), includeColor ? section()->style()->visitedDependentColor(startColorProperty) : Color(), BROWGROUP)); > if (!result.exists()) > - return result; > + return cellCollapsedBorders->start(result); This is globally an anti-pattern. You have to catch each and every |return| to properly cache your new value which makes the code hard to read and very sloppy. The computation should be moved to a new method (computeCollapsed*Border) and collapsed*Border should be the one querying the cache and recomputing as needed. > Source/WebCore/rendering/RenderTableCell.cpp:900 > void RenderTableCell::collectBorderValues(RenderTable::CollapsedBorderValues& borderValues) const > { > + ASSERT(table()->needsLayout()); If I am reading the code correctly, collectBorderValues can only be called during painting which makes this ASSERT very strange. > Source/WebCore/rendering/RenderTableCell.cpp:945 > - > + Unneeded & unrelated change. You are also not going far enough in your white space removal. > Source/WebCore/rendering/RenderTableSection.cpp:1347 > + return &result.first->second; Your logic is quite twisted here. I am not sure why it should be fine to request a collapsed border that is not in our cache. > Source/WebCore/rendering/RenderTableSection.h:133 > + CellCollapsedBorders* cellCollapsedBorders(const RenderTableCell*); You don't expect this function to return 0 or some garbage. It looks like it should return a reference in this case. > Source/WebCore/rendering/style/CollapsedBorderValue.h:82 > + CollapsedBorderValue end(CollapsedBorderValue end) { m_end = end; return m_end; } Setters should start with "set" per our coding style! It would make your change more readable. I am really not a huge fan of returning the value in those setters... > Source/WebCore/rendering/style/CollapsedBorderValue.h:87 > + bool exists() const { return m_start.exists() && m_end.exists() && m_before.exists() && m_end.exists(); } This function is unused.

(In reply to comment #9) > I would really love to have a comment explaining why *all* borders will be computed during layout. This would make sense as unfortunately there is no safe-guard against forgetting to compute a cell's border in your new caching code. I am not sure how difficult this would be to add so I am leaving this for you to see. Right, and since the intention is only to avoid recalculating the borders during painting I've changed the approach to be less prone to providing cached values when updated values are intended. Now paintCollapsedBorders() uses the cached values explicitly, in all other cases they are recalculated. I think this ensures that other callers will always get an up-to-date value for the border. If there is a layout or style change, the values will always get recomputed.

Created attachment 120612 [details] Patch

Comment on attachment 120612 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=120612&action=review It looks like some of the previous change got lost (the ASSERT for example, the change in RenderTable::layout, ...). > Source/WebCore/rendering/RenderTableCell.cpp:327 > - if (table && !table->selfNeedsLayout() && !table->normalChildNeedsLayout()&& oldStyle && oldStyle->border() != style()->border()) > + if (table && !table->selfNeedsLayout() && !table->normalChildNeedsLayout()) I don't think this change is right, this would invalidate our cached borders needlessly AFAICT. > Source/WebCore/rendering/RenderTableCell.cpp:397 > + ASSERT(cellCollapsedBorders.start().exists()); I fear that this ASSERT could be reached if you set "border-style: hidden" on a cell to disable collapsed borders or am completely wrong? > Source/WebCore/rendering/RenderTableCell.h:114 > + CollapsedBorderValue cachedCollapsedStartBorder() const; > + CollapsedBorderValue cachedCollapsedEndBorder() const; > + CollapsedBorderValue cachedCollapsedBeforeBorder() const; > + CollapsedBorderValue cachedCollapsedAfterBorder() const; I don't think we need yet others (collapsed) borders related functions. Why can't the "normal" functions be used and hide the implementation details that we cache the collapsed borders? > Source/WebCore/rendering/RenderTableCell.h:124 > + CollapsedBorderValue computeCollapsedStartBorder(IncludeBorderColorOrNot = IncludeBorderColor) const; > + CollapsedBorderValue computeCollapsedEndBorder(IncludeBorderColorOrNot = IncludeBorderColor) const; > + CollapsedBorderValue computeCollapsedBeforeBorder(IncludeBorderColorOrNot = IncludeBorderColor) const; > + CollapsedBorderValue computeCollapsedAfterBorder(IncludeBorderColorOrNot = IncludeBorderColor) const; Those should be private as don't want callers to call them needlessly. > Source/WebCore/rendering/RenderTableCell.h:135 > + CollapsedBorderValue cachedCollapsedLeftBorder() const; > + CollapsedBorderValue cachedCollapsedRightBorder() const; > + CollapsedBorderValue cachedCollapsedTopBorder() const; > + CollapsedBorderValue cachedCollapsedBottomBorder() const; > + Same question as with the other cachedCollapsed*Border functions.

(In reply to comment #12) > (From update of attachment 120612 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=120612&action=review > > It looks like some of the previous change got lost (the ASSERT for example, the change in RenderTable::layout, ...). Yes, the ASSERT was cruft (:/). The change in RenderTable::layout is no longer necessary because the use of cached collapsed border values now occurs only during painting, instead of depending on m_invalidCollapsedBorders being false when the caller requests the collapsed border values. > > > Source/WebCore/rendering/RenderTableCell.cpp:327 > > - if (table && !table->selfNeedsLayout() && !table->normalChildNeedsLayout()&& oldStyle && oldStyle->border() != style()->border()) > > + if (table && !table->selfNeedsLayout() && !table->normalChildNeedsLayout()) > > I don't think this change is right, this would invalidate our cached borders needlessly AFAICT. fast/table/simple_repaint.html is a case where updating the cell's background necessitates recalculating the collapsed borders. It might be possible to work out a specific set of style changes which require updating collapsed borders and test only for those but my worry is that I would miss one and break something. By updating for any style change to the cell I'm missing out on the performance gain for all style changes but I'm not worsening performance overall. The borders are cached for painting now, so where the style changes they are still only calculated once. The only downside is incurring the qsort for the border values on all style changes. I think this is less than the overhead of calculating the borders themselves as the number of unique collapsed borders to be sorted is generally very small since they are distinct only on style, width and precedence. > > > Source/WebCore/rendering/RenderTableCell.cpp:397 > > + ASSERT(cellCollapsedBorders.start().exists()); > > I fear that this ASSERT could be reached if you set "border-style: hidden" on a cell to disable collapsed borders or am completely wrong? You're right unfortunately. I think I can use the 'cache miss' return value from HashMap::add() to ensure that the cache contains something here. > > > Source/WebCore/rendering/RenderTableCell.h:114 > > + CollapsedBorderValue cachedCollapsedStartBorder() const; > > + CollapsedBorderValue cachedCollapsedEndBorder() const; > > + CollapsedBorderValue cachedCollapsedBeforeBorder() const; > > + CollapsedBorderValue cachedCollapsedAfterBorder() const; > > I don't think we need yet others (collapsed) borders related functions. Why can't the "normal" functions be used and hide the implementation details that we cache the collapsed borders? I understand your preference for this, but I think the it's important that the use of cached values for painting (vs computed values during layout or after a style change) is explicit here in order to avoid using cached values when a calculated value is needed. It seems fragile to depend on the state of m_invalidCollapsedBorders to enforce this, so I think it's safer to make it explicit when painting the borders. After all, during painting is when we expect to make the performance saving so better to not expose the cache to other users of the collapsed border functions. > > > Source/WebCore/rendering/RenderTableCell.h:124 > > + CollapsedBorderValue computeCollapsedStartBorder(IncludeBorderColorOrNot = IncludeBorderColor) const; > > + CollapsedBorderValue computeCollapsedEndBorder(IncludeBorderColorOrNot = IncludeBorderColor) const; > > + CollapsedBorderValue computeCollapsedBeforeBorder(IncludeBorderColorOrNot = IncludeBorderColor) const; > > + CollapsedBorderValue computeCollapsedAfterBorder(IncludeBorderColorOrNot = IncludeBorderColor) const; > > Those should be private as don't want callers to call them needlessly. > > > Source/WebCore/rendering/RenderTableCell.h:135 > > + CollapsedBorderValue cachedCollapsedLeftBorder() const; > > + CollapsedBorderValue cachedCollapsedRightBorder() const; > > + CollapsedBorderValue cachedCollapsedTopBorder() const; > > + CollapsedBorderValue cachedCollapsedBottomBorder() const; > > + > > Same question as with the other cachedCollapsed*Border functions.

Comment on attachment 120612 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=120612&action=review >>> Source/WebCore/rendering/RenderTableCell.cpp:327 >>> + if (table && !table->selfNeedsLayout() && !table->normalChildNeedsLayout()) >> >> I don't think this change is right, this would invalidate our cached borders needlessly AFAICT. > > fast/table/simple_repaint.html is a case where updating the cell's background necessitates recalculating the collapsed borders. It might be possible to work out a specific set of style changes which require updating collapsed borders and test only for those but my worry is that I would miss one and break something. By updating for any style change to the cell I'm missing out on the performance gain for all style changes but I'm not worsening performance overall. The borders are cached for painting now, so where the style changes they are still only calculated once. The only downside is incurring the qsort for the border values on all style changes. I think this is less than the overhead of calculating the borders themselves as the number of unique collapsed borders to be sorted is generally very small since they are distinct only on style, width and precedence. I am not sure I understand the issue with fast/table/simple_repaint.html. The cell's background color should not influence your collapsed borders and yet you seem to imply so. You make a very good point about the correctness and the performance impact. I would rather see the check adapted but at least adding a comment as to why you choose to do it this way would satisfy me. Make sure David Hyatt is fine with this change as he is the one who suggested it in https://bugs.webkit.org/show_bug.cgi?id=64546#c25 >>> Source/WebCore/rendering/RenderTableCell.cpp:397 >>> + ASSERT(cellCollapsedBorders.start().exists()); >> >> I fear that this ASSERT could be reached if you set "border-style: hidden" on a cell to disable collapsed borders or am completely wrong? > > You're right unfortunately. I think I can use the 'cache miss' return value from HashMap::add() to ensure that the cache contains something here. Your implementation has one logical flaw which shows up here is: you can't currently tell when you actually expect a cache miss or cache hit in your HashMap. It looks like you populate your cache only in one place (RenderTableCell::collectBorderValues) but your code doesn't use this information. I think your logic could be greatly simplified (also your assertions could be tightened) if you used that. >>> Source/WebCore/rendering/RenderTableCell.h:114 >>> + CollapsedBorderValue cachedCollapsedAfterBorder() const; >> >> I don't think we need yet others (collapsed) borders related functions. Why can't the "normal" functions be used and hide the implementation details that we cache the collapsed borders? > > I understand your preference for this, but I think the it's important that the use of cached values for painting (vs computed values during layout or after a style change) is explicit here in order to avoid using cached values when a calculated value is needed. It seems fragile to depend on the state of m_invalidCollapsedBorders to enforce this, so I think it's safer to make it explicit when painting the borders. After all, during painting is when we expect to make the performance saving so better to not expose the cache to other users of the collapsed border functions. This is a sound argument. Your ChangeLog needs to mention that by design your cache is only used during painting. I am still not satisfied with splitting totally the logic as this will induce some unneeded complexity, some copy & paste bugs and some bloat. Ideally we would like to reuse the existing functions by adding a parameter to say if you want to use the cache.

Created attachment 120813 [details] Patch

Hi Julien, I've slimmed the patch down quite a bit and stayed with the principle of enforcing cached border values when painting. A cache miss during painting will cause an assert, otherwise if a full border computation was requested the result will be cached.

Comment on attachment 120813 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=120813&action=review I like your new patch, it's cleaner and way more readable! The patch seems to be missing one part: removing the cache border values from the map when a cell is destroyed. This is a blocker as you could grow your memory without bounds in this approach. > Source/WebCore/rendering/RenderTableCell.cpp:548 > + section()->setCollapsedBorder(this, CBSBefore, result); What is the performance implication of doing a table lookup every time this code path is hit? (same for the other collapsedBeforeBorder) Wouldn't storing the new values during a collectCollapsedBorder achieve the same effect without the previous potential slow-down? (I am likely missing something here but I would like to know if the potential table lookup could be removed from the "common" code path) > Source/WebCore/rendering/RenderTableCell.cpp:969 > + CollapsedBorderValue leftVal = collapsedLeftBorder(IncludeBorderColor, UseCachedBorderValue); > + CollapsedBorderValue rightVal = collapsedRightBorder(IncludeBorderColor, UseCachedBorderValue); > + CollapsedBorderValue topVal = collapsedTopBorder(IncludeBorderColor, UseCachedBorderValue); > + CollapsedBorderValue bottomVal = collapsedBottomBorder(IncludeBorderColor, UseCachedBorderValue); If those are the only call sites where your new arguement is |UseCachedBorderValue| then it would be better to just inline this case here. > Source/WebCore/rendering/RenderTableSection.h:197 > + // It is held at TableSection level to spare memory consumption by table cells. s/TableSection/RenderTableSection/ (a section is a WebKit concept, the spec calls it row group) > Source/WebCore/rendering/RenderTableSection.h:198 > + HashMap<pair<const RenderTableCell*, int>, CollapsedBorderValue > m_cellCollapsedBorders; It should be m_cellsCollapsedBorders to be consistent.

Comment on attachment 120813 [details] Patch r- based on "The patch seems to be missing one part"

Comment on attachment 120813 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=120813&action=review >> Source/WebCore/rendering/RenderTableCell.cpp:548 >> + section()->setCollapsedBorder(this, CBSBefore, result); > > What is the performance implication of doing a table lookup every time this code path is hit? (same for the other collapsedBeforeBorder) > > Wouldn't storing the new values during a collectCollapsedBorder achieve the same effect without the previous potential slow-down? (I am likely missing something here but I would like to know if the potential table lookup could be removed from the "common" code path) Unfortunately this patch doesn't 'only compute the border when calling collectBorderValues', it only does: 'use the cached border values when painting'. 'Only compute in collectBorderValues' is the next step after this but it's quite tricky, as the borders are often computed/updated outside layout. I'm not even sure it will be possible to achieve in a satisfactory way so I have started with this as a performance gain in its own right.

Comment on attachment 120813 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=120813&action=review >> Source/WebCore/rendering/RenderTableCell.cpp:969 >> + CollapsedBorderValue bottomVal = collapsedBottomBorder(IncludeBorderColor, UseCachedBorderValue); > > If those are the only call sites where your new arguement is |UseCachedBorderValue| then it would be better to just inline this case here. These are indirect calls to collapsed[Before|After|Start|End]Border depending on writing mode, so inlining would require creating similarly named functions with hardcoded parameters calling into collapsed[Before|After|Start|End]Border, no? Given the indirection I can't see a good way of doing this that doesn't create four new unnecessary-looking functions so I must be missing your point here!

(In reply to comment #20) > > These are indirect calls to collapsed[Before|After|Start|End]Border depending on writing mode, so inlining would require creating similarly named functions with hardcoded parameters calling into collapsed[Before|After|Start|End]Border, no? Given the indirection I can't see a good way of doing this that doesn't create four new unnecessary-looking functions so I must be missing your point here! Dumb response - please ignore.

Created attachment 121550 [details] Patch

Created attachment 121551 [details] Patch

(In reply to comment #23) > Created an attachment (id=121551) [details] > Patch Hi Julien, See the ChangeLog for the changes I've made to address your comments.

Comment on attachment 121551 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121551&action=review > Source/WebCore/rendering/RenderTableSection.cpp:1345 > + for (int side = CBSBefore; side < CBSEnd; ++side) Whoops, that should be <=.

Created attachment 121552 [details] Patch

Comment on attachment 121552 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121552&action=review I am fine with the new patch. I would love to see the potential performance degradation measured or at least investigated before giving the final r+ and added to the ChangeLog (also the performance benefit would be nice to give somewhere). > Source/WebCore/rendering/RenderTableCell.cpp:401 > + if (includeColor) > + section()->setCollapsedBorder(this, CBSStart, result); Adding a table lookup here could slow down some operations. Have you checked that it does not make us regress some of the already optimized code paths? (see https://bugs.webkit.org/show_bug.cgi?id=74813 for an example) > Source/WebCore/rendering/RenderTableCell.cpp:716 > +inline CollapsedBorderValue RenderTableCell::collapsedLeftBorder(RenderStyle* tableStyle) const > { > - RenderStyle* tableStyle = table()->style(); > if (tableStyle->isHorizontalWritingMode()) > - return tableStyle->isLeftToRightDirection() ? collapsedStartBorder(includeColor) : collapsedEndBorder(includeColor); > - return tableStyle->isFlippedBlocksWritingMode() ? collapsedAfterBorder(includeColor) : collapsedBeforeBorder(includeColor); > + return tableStyle->isLeftToRightDirection() ? collapsedStartBorder(IncludeBorderColor, UseCachedBorderValue) : collapsedEndBorder(IncludeBorderColor, UseCachedBorderValue); > + return tableStyle->isFlippedBlocksWritingMode() ? collapsedAfterBorder(IncludeBorderColor, UseCachedBorderValue) : collapsedBeforeBorder(IncludeBorderColor, UseCachedBorderValue); > } You could inline your cache checks here (section()->cachedCollapsedBorder(this, ...);) and remove a branch from the common case. I don't mind keeping the argument in collapsed*Border as this could enable more call-sites to use it. > Source/WebCore/rendering/RenderTableSection.cpp:1345 > + for (int side = CBSBefore; side <= CBSEnd; ++side) s/int/CollapsedBorderSide/ ?

(In reply to comment #27) > > > Source/WebCore/rendering/RenderTableCell.cpp:401 > > + if (includeColor) > > + section()->setCollapsedBorder(this, CBSStart, result); > > Adding a table lookup here could slow down some operations. Have you checked that it does not make us regress some of the already optimized code paths? (see https://bugs.webkit.org/show_bug.cgi?id=74813 for an example) The only code paths that compute a border and look up the cache (in the process of setting it) are style change, layout, and calculation of the table's width. The optimized path introduced in 74813 is unaffected - it still just computes a border and returns it. The big, and only, winner is paintCollapsedBorders() which never computes a border now - it just does a straight cache lookup. There are no other paths to this code that I'm aware of. So it seems like a straightforward performance gain to me.

Created attachment 122053 [details] Patch

Comment on attachment 122053 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122053&action=review > Source/WebCore/ChangeLog:26 > + Compute, and also cache if the full border including color was computed. Compute, and cache _ if ... (looks like a noun is missing and should be put instead of the placeholder (disclaimer, I am a non-native English speaker)) > Source/WebCore/rendering/RenderTableCell.cpp:63 > + recalcSection->removeCellCollapsedBorders(this); You should check that the table has collapsing borders before calling removeCellCollapsedBorders (adding 4 table lookups for nothing). > Source/WebCore/rendering/RenderTableCell.h:176 > + CollapsedBorderValue collapsedLeftBorder(RenderStyle*) const; > + CollapsedBorderValue collapsedRightBorder(RenderStyle*) const; > + CollapsedBorderValue collapsedTopBorder(RenderStyle*) const; > + CollapsedBorderValue collapsedBottomBorder(RenderStyle*) const; Those should be renamed cachedCollapsed*Border as they use in the cache now. > Source/WebCore/rendering/RenderTableSection.cpp:1346 > + m_cellsCollapsedBorders.take(make_pair(cell, side)); This should be m_cellsCollapsedBorders.remove as we don't need the return value from take. > Source/WebCore/rendering/RenderTableSection.cpp:1356 > + HashMap<pair<const RenderTableCell*, int>, CollapsedBorderValue>::iterator it = m_cellsCollapsedBorders.find(make_pair(cell, side)); All the 3 previous functions should ASSERT that our table has collapsing borders. > Source/WebCore/rendering/RenderTableSection.h:141 > + void removeCellCollapsedBorders(const RenderTableCell*); > + void setCollapsedBorder(const RenderTableCell*, CollapsedBorderSide, CollapsedBorderValue); For consistency, those 2 functions should be named: removeCachedCellCollapsedBorders and setCachedCollapsedBorder. If you have more consistent and better naming, feel free to use it (I am not entirely satisfied by the 2 previous names).

Committed r105029: <http://trac.webkit.org/changeset/105029>

After this change when using fixed layout some pages hit an assert in RenderTableSection.cpp: https://bugs.webkit.org/show_bug.cgi?id=76405