Tree body frame could cache row count

RESOLVED FIXED

Status

()

--
enhancement
RESOLVED FIXED
15 years ago
10 years ago

People

(Reporter: neil, Assigned: neil)

Tracking

({perf})

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

15 years ago
The tree body frame recently acquired a new member variable mCountBeforeUpdate.
This variable (suitably renamed) could be used to cache the row count. It would
only be updated by RowCountChanged and EndUpdateBatch. This would save 13 calls
to GetRowCount.

Comment 1

15 years ago
Yeah, I agree
Neil, do you want to take it ?
(Assignee)

Comment 2

15 years ago
Taking.
Assignee: varga → neil.parkwaycc.co.uk
(Assignee)

Comment 3

15 years ago
Created attachment 124911 [details] [diff] [review]
Proposed patch

Tested with a content view, an rdfliner and a custom view.
(Assignee)

Updated

15 years ago
Attachment #124911 - Flags: review?(varga)

Comment 4

15 years ago
Comment on attachment 124911 [details] [diff] [review]
Proposed patch

Comments on the patch:
1. Feel free to change mUpdateBatchNest from PRInt16 to PRInt32. Originally, I
wanted to couple it with other booleans, but it seems it's not worth trying.

2. I've noticed that you included some other changes in your patch, in
EnsureView() and in some of the scrolling methods. Could you extract them and
include them in a new bug e.g. "Tree enhancements" ?
I'll review those later

Other than that looks good.
Attachment #124911 - Flags: review?(varga) → review-
(Assignee)

Comment 5

15 years ago
Created attachment 126029 [details] [diff] [review]
Bits chopped out

I chopped out my patches to two other bugs (not enhancements) as requested.
I put in an extra assert to catch some other abusers of trees.
Attachment #124911 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #126029 - Flags: review?(varga)

Comment 6

15 years ago
Comment on attachment 126029 [details] [diff] [review]
Bits chopped out

>@@ -738,6 +733,7 @@
>     }
> 
>     // The scrollbar will need to be updated.
>+    mView->GetRowCount(&mRowCount);
>     InvalidateScrollbar();

Could this be moved above the comment with an extra blank line?

>@@ -892,6 +888,11 @@
>   if (!mRect.IsEmpty()) {
>     nsLeafBoxFrame::Invalidate(mPresContext, mRect, PR_FALSE);
>   }
>+  if (mView) {
>+    PRInt32 rowCount = mRowCount;
>+    mView->GetRowCount(&mRowCount);
>+    NS_ASSERTION(mRowCount == rowCount, "row count changed unexpectedly");
>+  }

I think this can be ifdef'ed:
+
+#ifdef DEBUG
+  if (mView) {
+    PRInt32 rowCount = mRowCount;
+    mView->GetRowCount(&mRowCount);
+    NS_ASSERTION(mRowCount == rowCount, "row count changed unexpectedly");
+  }
+#endif
+
Attachment #126029 - Flags: review?(varga) → review+
(Assignee)

Comment 7

15 years ago
Comment on attachment 126029 [details] [diff] [review]
Bits chopped out

I did think about #ifdef but I was worried that this would result in different
behaviour in debug and release builds.
Attachment #126029 - Flags: superreview?(jaggernaut)

Comment 8

15 years ago
ah, it's in Invalidate()
Actually, why do we need to refresh mRowCount there ?
If you want to just catch bad callers, add |#ifdef DEBUG| and compare it without
refreshing mRowCount. Otherwise I think some code could depend on this in future.

+
+#ifdef DEBUG
+  if (mView) {
+    PRInt32 rowCount;
+    mView->GetRowCount(&rowCount);
+    NS_ASSERTION(mRowCount == rowCount, "row count changed unexpectedly");
+  }
+#endif
+
(Assignee)

Comment 9

15 years ago
Jan, nsXULTreeBuilder.cpp abuses the row count :-(
I'm not sure if I'll be able to figure out why...
(Assignee)

Comment 10

15 years ago
My bad, EndUpdateBatch calls Invalidate() :-[

Comment 11

15 years ago
If this is the case then we better move it to Paint() method.
(Assignee)

Comment 12

15 years ago
Jan: here's a good one: when you remove a <tree>'s immediate <treechildren> node
then the tree content view simply calls Invalidate() - it does NOT call
RowCountChanged()! I'm surprised it works at all.

Comment 13

15 years ago
As I said, just move it to the Paint() method for now (maybe w/o the assestion).
I'm ok with that.
(Assignee)

Comment 14

15 years ago
I'm not quite sure what you want:
1) Leave some or all of the GetRowCount()s in for now
2) As 1) but in debug builds only; release builds always use the cached count
3) As 1) but assert if the count changed
4) Both 2) and 3)
(Assignee)

Comment 15

15 years ago
OK, so it doesn't assert in Paint(), so I guess I have no issues with the
content or builder views. I'll leave the #ifdef'd assert in Paint() though, just
in case something else crops up.

Comment 16

15 years ago
sounds good
(Assignee)

Comment 17

15 years ago
Created attachment 126459 [details] [diff] [review]
Moved and #ifdef'd the asserts
(Assignee)

Updated

15 years ago
Attachment #126459 - Flags: superreview?(jaggernaut)
Attachment #126459 - Flags: review?(varga)

Updated

15 years ago
Attachment #126459 - Flags: review?(varga) → review+

Comment 18

15 years ago
Comment on attachment 126459 [details] [diff] [review]
Moved and #ifdef'd the asserts

+#ifdef DEBUG
+  PRInt32 rowCount = mRowCount;
+  mView->GetRowCount(&mRowCount);
+  NS_ASSERTION(rowCount == mRowCount, "row count did not change by the amount
suggested, check caller");
+#endif

So if anything's wrong, DEBUG will behave differently than non-DEBUG because it
just updated mRowCount. Of course, it will assert about it, so it may be okay.
I think I'd still prefer mView->GetRowCount(&rowCount) though (both cases).

sr=jag with that changed
Attachment #126459 - Flags: superreview?(jaggernaut) → superreview+

Updated

15 years ago
Attachment #126029 - Flags: superreview?(jaggernaut)
(Assignee)

Comment 19

15 years ago
Now I remember why I update the row count after asserting - if you don't then
every time you paint you'll assert! This isn't a problem in RowCountChanged
though, that can go either way as you prefer.
(Assignee)

Comment 20

15 years ago
I updated the RowCountChanged assert for bitrot and jag's comment.
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
(Assignee)

Comment 21

15 years ago
Didn't quite fix the bitrot properly :-( New patch coming up.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 22

15 years ago
Created attachment 126818 [details] [diff] [review]
Corrected merge
(Assignee)

Updated

15 years ago
Attachment #126818 - Flags: superreview?(jaggernaut)
Attachment #126818 - Flags: review?(varga)
(Assignee)

Comment 23

15 years ago
Turns out that adam moved some code as part of bustage so I moved it back :-P
Status: REOPENED → RESOLVED
Last Resolved: 15 years ago15 years ago
Resolution: --- → FIXED
(Assignee)

Updated

15 years ago
Attachment #126818 - Flags: superreview?(jaggernaut)
Attachment #126818 - Flags: review?(varga)

Updated

15 years ago
OS: Windows 2000 → All
Hardware: PC → All

Comment 24

15 years ago
Comment on attachment 126818 [details] [diff] [review]
Corrected merge

Erh...

+  PRInt32 rowCount = mRowCount;
+  mView->GetRowCount(&rowCount);

Didn't you want to |GetRowCount(&mRowCount)| there?

Nit: in the other check you assert on mRowCount == rowCount, here it's the
other way around.
(Assignee)

Comment 25

15 years ago
If you do change the row count when you assert on change, then if someone
precomputes the final row count but calls RowCountChanged multiple times then
the last change would fire an assertion, despite the row count being correct.
If you don't change the row count when you assert on paint, then you'll keep
asserting on paint. Actually now that DOM Inspector is fixed I should be able to
get away with changing this as it shouldn't annoy anybody.

Updated

10 years ago
Component: XP Toolkit/Widgets: Trees → XUL
QA Contact: shrir → xptoolkit.widgets
You need to log in before you can comment on or make changes to this bug.