GetPosition and GetPositionSlowly call to the prefs service _every_time_

NEW
Unassigned

Status

()

Core
Selection
P3
normal
18 years ago
9 years ago

People

(Reporter: shaver, Unassigned)

Tracking

({perf})

Trunk
mozilla1.1alpha
x86
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

...which just seems wasteful.

I considered a couple of solutions to this:

1) cache the pref value on the frame, and add an updater

   Not very good: you'd have tons of pref updaters, and it'd cost us 4 bytes
   per nsTextFrame object

2) cache the pref value in a static value, and install an updater the first time
   through the code (or until it works, if it fails)

   Much better.  I'll attach a patch to that effect.
Created attachment 18258 [details] [diff] [review]
Cache and update pref state in nsTextFrame::gDragOutOfFrameStyle

Comment 2

18 years ago
The patch mostly looks good (thanks for doing this -- should help a lot!), but I
have one concern: in the embedded case, where we don't necessarily have a pref
service, we'll keep calling SetDragOutOfFrameStyleUpdater() every time.  It
might be better to just assume one setting or the other (I'd vote for the
Mac/Unix setting because more platforms use it and I wonder whether Windows
people really *like* the Windows setting ...) in the case where we can't get the
pref service, rather than leaving it undefined so we try again next time.

Updated

17 years ago
Assignee: mjudge → anthonyd
Target Milestone: --- → mozilla0.9

Comment 3

17 years ago
reassign to anthonyd, setting to moz0.9, anthony can you modify the patch based 
on Akkana's input and get this one reviewed and checked in?

Comment 4

17 years ago
can do!

anthonyd
Status: NEW → ASSIGNED

Comment 5

17 years ago
okay, i got shaver's patch with akkana's changes in my tree.  i will diff it up 
and attach it shortly for r= and sr='s.

anthonyd
Whiteboard: FIX IN HAND

Comment 6

17 years ago
Created attachment 26564 [details] [diff] [review]
new diff with shavers stuff, and akkana changes

Comment 7

17 years ago
Created attachment 26566 [details] [diff] [review]
BAH! This is the right patch

Comment 8

17 years ago
Cc'ing alecf. Alec, you said that nsIPref::RegisterCallback will eventually
disappear. Would you please take a look at this patch? They are registering a
callback to update a global variable (and not unregistering the callback at
all, since that variable is global, after all). In this kind of case, would we
really want to create an nsIObserver on the heap and store a pointer to it in
a static variable, just so that we can call addObserver/removeObserver? Or will
we end up keeping RegisterCallback around for these types of situations? Also,
if RegisterCallback really is going to be removed, shouldn't there be a comment
to that effect in nsIPref.idl?

Also, the patch doesn't seem to address Akkana's concern (embeddor may not have
prefs), even though you say that you addressed her concern.

Comment 9

17 years ago
normally you might just figure out what global service might actually own the
global variable, and then make that the observer

in this case though, it does look like the right thing is to just make a single
local observer to observe this topic..

Comment 10

17 years ago
setting to mozilla 0.8.1
anthonyd
Target Milestone: mozilla0.9 → mozilla0.8.1

Comment 11

17 years ago
moving to mozilla0.9
Target Milestone: mozilla0.8.1 → mozilla0.9

Comment 12

17 years ago
I should add that I think that in the name of improving performance ASAP, we
should just use RegisterCallback for now.

Comment 13

17 years ago
Moving to mozilla0.9.1.
Whiteboard: FIX IN HAND
Target Milestone: mozilla0.9 → mozilla0.9.1

Comment 14

17 years ago
moving to 0.9.2
Target Milestone: mozilla0.9.1 → mozilla0.9.2

Comment 15

17 years ago
im gonna reassign this to shaver since everyone but me and him doesnt seem to 
like the patch.

anthonyd
Assignee: anthonyd → shaver
Status: ASSIGNED → NEW
Asa just told me that we shipped 0.9.2!  Who knew?
Target Milestone: mozilla0.9.2 → mozilla0.9.3

Updated

17 years ago
Keywords: perf

Updated

17 years ago
Target Milestone: mozilla0.9.3 → mozilla0.9.4

Comment 17

17 years ago
What about simply caching this in the presentation context and using the
nsIPresContext::GetCachedBoolPref() stuff?

Updated

17 years ago
Target Milestone: mozilla0.9.4 → mozilla0.9.5

Comment 18

17 years ago
->0.9.5 since we have run out of time

Comment 19

17 years ago
I recently added a sibling nsIPresContext::GetCachedIntPref() if preferred here.
I'm going to give this back to anthonyd, to either fix or WONTFIX at his
leisure.  I don't have time to chase this any more.
Assignee: shaver → anthonyd
Target Milestone: mozilla0.9.5 → ---

Comment 21

16 years ago
changing selection qa to tpreston.
QA Contact: blaker → tpreston

Comment 22

16 years ago
reassigning to mjudge.  mike come talk to me about this one if you want.
Assignee: anthonyd → mjudge

Updated

16 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.1alpha
Assignee: mjudge → nobody
Status: ASSIGNED → NEW
QA Contact: tpreston → selection
You need to log in before you can comment on or make changes to this bug.