Closed Bug 105445 Opened 23 years ago Closed 23 years ago

Difficult to process PLEvents without either latency or CPU hogging

Categories

(Core Graveyard :: Embedding: APIs, defect)

Product:
Core Graveyard
Component:
Embedding: APIs
Platform:
PowerPC
Mac System 9.x
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
mozilla1.0

People

(Reporter: ccarlen, Assigned: ccarlen)

References

Details

(Keywords: topembed+)

Attachments

(1 file, 1 obsolete file)

new patch
4.12 KB, patch
mikepinkerton
: review+
sfraser_bugs
: superreview+
asa
: approval+
Details | Diff | Splinter Review
PPEmbed has always just passed 0 as the sleep time to WaitNextEvent because it was quick and dirty and better calculation of the sleep time would always have to be done by the embedding app or its framework. With a sleep time of zero, a debug build comes up with 4584 for the average of all pages. I added some code to EmbedEventHandling.cpp to set the sleep time to 0 only when Gecko was busy, resetting it to the app's initial value when not busy. With that code, PPEmbed no longer pegs the CPU when idle - it fluctuates beween 0 and 3 (roughly) when idle. It did the page loader test in 5565 - quite a bit worse. Also, to do this, I had to use nsIEventQueue and nsIEventQueueService which we don't want in embedding client code. The 2nd attempt was to not bother with dynamically setting the sleep time at all. Instead, I just made _pl_NativeNotify in plevent.c call WakeUpProcess. This brought the page loader average down to 4580 (best of all!) and that was with the sleep time of the app left at 15 and never twiddled at all.
QA Contact: mdunn → depstein
Attached patch patch to wake us up (obsolete) — — Splinter Review
When the sleep time is set to a longer value, scrolling slows downs. Our scrolling code on the Mac is dependent on timers which are serviced at idle time. I still had to have some code in the embedding app which knocked the sleep time down to 0 when the mouse was down. Since that's a simple toolbox call, it's not as evil for the embedding app as looking at event queues, but it would be nice to make that unnescesary as well.
If we want to call _pl_NativeNotify() from code that can run at interrupt time (e.g. OT notifiers), does 'kCurrentProcess' do the right thing?
Depends on whether kCurrentProcess means "the process of the code being executed right now" or "the current process in terms of the process manager." Because of this, it would be better find out the process ID when we are known to be not at interrupt time and store it.
The approach in the patch may become obsolete if we move to Carbon events. If we don't do that before 0.9.7, I'll do this.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.7
-> 0.9.8
Target Milestone: mozilla0.9.7 → mozilla0.9.8
> The approach in the patch may become obsolete if we move to Carbon events. With Carbon events, _pl_NativeNotify could fire off a custom event which, when processed, just calls PL_ProcessPendingEvents. This is pretty much what Windows does. Changing summary because this problem isn't specific to PPEmbed - general problem with PLEvent handling on the Mac.
Summary: PPEmbed is a CPU hog → Difficult to process PLEvents without either latency or CPU hogging
is this now the same as bug 106691?
No - unless processing PLEvents is now done using timer tasks (I don't think so)
pav's new timer impl posts a PLEvent when the timer fires. so instead of polling to see if anything is in the PLEvent queue, we should just use that event posting mechanism you suggested...or am i still way off here?
This patch is a simple way to process PLEvents with less latency and without tricky calculation of WNE sleep time when using Classic events. The PLEvents posted by pav's timer tasks would benefit from this just the same as other PLEvents.
right, so we agree ;) it seems we need to do a couple of things: - apply this patch - remove the WNE sleep param setting code since it's no longer necessary, right? - don't poll the PLEvent queue on idle. With the above work, 106691 is a dupe of this, and I'll mark it as such. Do you have time to finish this off, or should I take this bug and get it ready for landing?
*** Bug 106691 has been marked as a duplicate of this bug. ***
-> 0.9.9
Target Milestone: mozilla0.9.8 → mozilla0.9.9
Attached patch new patch — — Splinter Review
The newer patch is the same idea as the first but it gets the current process ID once when we know we're not being called from an interrupt and stores it. It removes all the stuff from the msg pump which calculates sleep time. The good part of this is that we really don't want embedding apps to have to do what BrowserIsBusy() does. Here are some perf numbers done with a debug build on an iBook using jrgm's cowtools test: Without patch: Avg. Median : 3502 msec With patch, sleep time = 8: Avg. Median : 3661 msec With patch, sleep time = 5: Avg. Median : 3556 msec I would have hoped that using WakeupProcess(), we could just pass LONG_MAX for the sleep time. Doesn't appear to be so. Performance still rises as sleep time goes down. Using a sleep time of 5, the perf comes close to the current scheme. However, we're using much less % of the CPU in doing so and embedding apps, which don't muck with the sleep time, will benefit. Anybody care to try this patch and see how your numbers are?
Attachment #54068 - Attachment is obsolete: true
Blocks: 127253
Some new timings on an optimized build: Mozilla Without patch: Avg. Median : 2206 msec With patch, sleep time = 5: Avg. Median : 2207 msec PPEmbed Without patch, sleep time = 5 Avg. Median : 2385 msec With patch, sleep time = 5: Avg. Median : 2082 msec That's a decent improvement for PPEmbed. Given the timing data, the only thing that makes me < 100% sure about this is that drawing seems to happen in larger chunks during page loading. It's hard to say. Simon, Pink, can you apply this and see what you think?
conrad, the easiest way to tell if this is doing the right thing or starving updates is to use the instrumentation sdk. Instrument the HandleUpdate routine and the PLEvent processing routine. If they are intermingled, then it's ok. If all PLEvents are processed before any update event, that's bad. Do you mind taking a quick look just to verify? I think the fact that mozilla stand-alone isn't affected means that it's probably ok. Also note that WakeUpProcess() does nothing in the world where we're not using WNE (it's a no-op). When we move to full carbonevents, this will have to be revisited to post a carbon event (which apple has assured us will no longer starve updates in the carbonEvent world...yay!).
Nominating topembed.
Keywords: topembed
Target Milestone: mozilla0.9.9 → mozilla1.0
i ran this test with a 3/12/02 opt build (osx) and got the following numbers: o without patch: 1661ms o with patch: 1679ms about a 1% slowdown for mozilla. could be network fluxuations across the country (dulles to mtnview). If we do get 10% speedups with embedded apps, I say go for it. How did the instrumentation graphs come out?
The instrumentation graphs showed, if anything, less starving of update events. I instrumented HandleUpdate and ProcessPLEventQueue and, with the patch, there were slightly more updates events though they tended to be shorter. I'm not sure how much faith I have in those graphs in this case - graphs of the same build tended to vary almost as much as ones with patch vs. without patch. I'd say the bottom line is the numbers from the page loader tests. I say we go for it, too. Pink, Simon, can I get r=/sr=?
Comment on attachment 69301 [details] [diff] [review] new patch whereever you add an XP_MAC, also add XP_MACOSX for mach-o. r=pink
Attachment #69301 - Flags: review+
How does this affect CPU load while a pageload is in progress? Do we hog the CPU more than before? Does it affect responsiveness of foreground apps in 9, when we're in the background?
> How does this affect CPU load while a pageload is in progress? Currently, in Seamonkey, we use nearly 100% of the CPU while loading a page, so we're not any worse off. The nice part is that, with the patch, when not loading a page, we're using much less CPU.
Comment on attachment 69301 [details] [diff] [review] new patch sr=sfraser
Attachment #69301 - Flags: superreview+
Does this patch affect bug 124305?
I would have to do another Classic build to see. Though I tested autoscrolling with the patch on Classic, I didn't happen to try a case which would repro that bug. I doubt this patch would help because, when the mouse is down, we're passing a sleep time of 0 with or without the patch.
Comment on attachment 69301 [details] [diff] [review] new patch a=asa (on behalf of drivers) for checkin to the 1.0 trunk
Attachment #69301 - Flags: approval+
EDT Triage (chofmann et al) Has this patch been checked in?
Keywords: topembed → topembed+
Checked in. Pink, as far as comment #21, there was already this bit in plevent.c which takes care of that. #if defined(XP_MACOSX) #undef XP_UNIX #define XP_MAC 1 #endif
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Those #ifdefs look like a really bad idea. No-one should #define XP_MAC in code.
> Those #ifdefs look like a really bad idea. pavlov added that about 2 years ago > No-one should #define XP_MAC in code. If you want a whole unit of code which is being built for Mach-0 to use Mac APIs instead of XP_UNIX, it makes sense - the "whole unit" being the key here. If that was not done, I would have had to add a huge number of separate #ifdef all over he place throughout the file.
Using 'top', general range of PPEmbed cpu usage is 3-5 percent on OSX (Mac G4). this bug fixed before we had packaged builds.
Status: RESOLVED → VERIFIED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: