generic request prioritization (loadgroup prioritization) this bug is a spin-off from bug 142255. to make the HTTP channel prioritization easier to use, we should allow a load priority to be assigned to loadgroups and have that affect all requests contained in the loadgroup. this probably means implementing a common interface like: interface nsIPriorityRequest : nsISupports { attribute short priority; }; then loadgroups, HTTP channels, and image requests would support this interface. from bug 142255 comment #22: "loadgroup::addrequest would do something like: aRequest.priority += mPriority; instead of =." and it would probably make sense for loadgroup::removeRequest to undo that (in case a request is being moved from one loadgroup to another). instead of introducing nsIPriorityRequest, we could also make use a named properties interface such as the one I proposed in bug 270224.

Boris and I came up with nsIScheduled as the interface of choice for this. It seemed to us that there is no reason to tie the inteface to network requests, as it could very well be used to express scheduling prioritization of other sorts of objects, such as threads or processes. This patch makes nsHttpChannel, imgRequestProxy, nsLoadGroup, and nsDocLoader all implement nsIScheduled. I also make NewImageChannel set a default priority of LOW for images. It was really neat to visit www.gamespot.com, click to open a new tab on some content while the page was still loading, and see the new page (the HTML at least) load before the old one finished loading :)

NOTE: the point of implementing nsIScheduled on nsDocLoader is to make it easy to set a load priority on a "docshell" from the tabbrowser code. The plan is to make it so that the current tab has a higher priority than background tabs.

Comment on attachment 171611 [details] [diff] [review] v1 patch this is really cool. a few comments right now: - nsIHttpChannelInternal: it would be nice if it got the same IID it had before the priority adding (uuid(f3764874-ed7e-4873-883c-11d67a4e3638)) netwerk/base/src/nsLoadGroup.cpp + else if (aIID.Equals(NS_GET_IID(nsIScheduled))) { + *aInstancePtr = NS_STATIC_CAST(nsIScheduled*,this); missing space before |this| Would it make sense for nsSocketTransport to implement nsIScheduled as well? For example, would reordering the mPollList/mActiveList in the socket transport service make earlier connections faster?

yeah, i agree.. i should revert the uuid of that interface. as for the socket transport service, i didn't think it mattered where a socket was placed in the poll list. do you know of any OS documentation that can shed some light on the matter?

so, there are three other things out there that deal with schedules. bookmark updates, rss feeds and calendars. if you're making an interface which doesn't benefit or accurately describes scheduling in any of those places, then we have a problem. at a quick glance, nsIScheduled most certainly doesn't fit with the bookmarks view of scheduling which is more about how often or when to check for something and less about prioritizing things. perhaps you should consider the way mailnews exposes message priorities, since that's a much closer fit.

interesting... perhaps some things are better expressed using a different interface. this interface is meant to capture the OS notion of scheduling tasks (see "man 2 setpriority" or "man 2 nice"). i'm not sure we want to try to fit all those shoes into the same boot here.

nsIPriority sounds fine to me, i could even use that in the mail case. I don't understand why you picked Scheduled over Priority, the interface clearly deals in priorities, as noted it does so generically, but in terms of priorities, not schedules....

or nsIPrioritized i suppose

nsIScheduled was really just in response to "nsIPrioritizable" which was just too painful to type ;-) nsIPrioritized is better than nsIPrioritizable, so I could go with that too I guess.

nsISupportsPriority ;-)

Comment on attachment 171611 [details] [diff] [review] v1 patch Other than timeless's comment about Interface name, my only thought is with the const in the idl >Index: xpcom/threads/nsIScheduled.idl >=================================================================== >+ /** >+ * Typical priority values. >+ */ >+ const long HIGHEST = -20; >+ const long HIGH = -10; >+ const long NORMAL = 0; >+ const long LOW = 10; >+ const long LOWEST = 20; Perhaps a + const long FIRST = -20; + const long LAST = 20; + const long STEP = 10; or some such, incase a caller wishes to enumerate the priorities for some reason, or do range checking. I'm not too sure how useful it would be, but would save a caller breaking incase someone tries to do that at some point. Nice work, I like ;-)

(In reply to comment #4) > as for the socket transport service, i didn't think it mattered where a socket > was placed in the poll list. I have no idea whether it does :-) hm, thinking about it, it seems really unlikely that it does.

> or some such, incase a caller wishes to enumerate the priorities for some > reason, or do range checking. Such a caller has clearly not read the comments on this interface very well. Any 32-bit integer value is a valid priority value, a-priori.

To anyone who cares about the name of this interface, please weigh in with your opinion by tomorrow. I'll decide based on feedback at that time and then cut a new patch. Thanks!

bz, makes sense to me then I guess Darin, nsIPrioritized or nsISupportsPriority sounds good to me, no clue on if there is anything better.

nsISupportsPriority sounds rather bad to me... I'd prefer nsIPrioritized, I think.

Comment on attachment 171611 [details] [diff] [review] v1 patch netwerk/protocol/http/src/nsHttpChannel.cpp + PRInt16 newValue = CLAMP(value, PR_INT16_MIN, PR_INT16_MAX); is there a reason for this? the interface allows it, obviously, but why truncate the value? (is it to make the object a bit smaller?) modules/libpr0n/src/imgLoader.cpp so... I think this is OK... but I wonder if it should maybe be nsContentUtils::LoadImage that does this scheduling? (almost the only LoadImage caller) I'm undecided... feel free to leave it here :-) pavlov should probably OK the imagelib changes, though. uriloader/base/nsDocLoader.cpp +NS_IMETHODIMP nsDocLoader::SetPriority(PRInt32 aPriority) +{ + // Set the priority of our loadgroup, and then set the priority + // of each child nsDocLoader instance. hm... that second part can be removed once bug 261091 is fixed + if (mLoadGroup) { + nsCOMPtr<nsIScheduled> scheduled = do_QueryInterface(mLoadGroup); do_QI is nullsafe... no need for the if + loader = NS_STATIC_CAST(nsDocLoader*, ChildAt(i)); + if (loader) { + loader->SetPriority(aPriority); are children of a docloader ever null? So question... when one sets a priority on a docloader, and a new load starts. shouldn't the set priority be used for that too? hm... ok, this may do the right thing, as the loadgroup is reused (I think); but, it won't work for child docloaders. in fact, new children don't get the stored priority at all. is this worth fixing? Bug 261091 would automatically fix it. r=me, with whatever name you want to pick :-)

the interface really should go in xpcom/ds, xpcom/threads, or xpcom/io. It might make sense to expose this on object in xpcom as well. I like the name nsIPriority, myself, as the only attribute of this interface is named |priority|.

> the interface really should go in xpcom/ds, xpcom/threads, or xpcom/io. It > might make sense to expose this on object in xpcom as well. doug: thanks for the feedback. right now, i have the interface in xpcom/threads, which seems to make sense since it is modeled after "classical" OS thread priorities.

Comment on attachment 171611 [details] [diff] [review] v1 patch >Index: xpcom/threads/nsIScheduled.idl I have no strong opinions on the name, so whatever the final decision on this was... ;) >Index: netwerk/protocol/http/public/nsIHttpChannelInternal.idl >-[scriptable, uuid(ae9dce68-c27c-44f1-b41d-462f5e470945)] >+[scriptable, uuid(6f225cf9-dfeb-4b9f-8ff5-2a5bf31b73a1)] Actually, you're restoring the file back to what it was in rev 1.5. Want to just restore that iid as well? That would make the most sense to me. >Index: modules/libpr0n/src/imgLoader.cpp I'm not so happy with this part. Could you ok it with Stuart? Does this leave it possible to load images at the desired priority for imagelib users who don't want LOW? Doing this in nsContentUtils instead like biesi suggests would make a lot of sense, if that would work. It seems to me that it should. >Index: modules/libpr0n/src/imgRequestProxy.cpp >+NS_IMETHODIMP imgRequestProxy::GetPriority(PRInt32 *priority) >+{ >+ NS_ENSURE_STATE(mOwner); >+ *priority = mOwner->Priority(); >+ return NS_OK; So... this can leave priority uninitialized (if mOwner is null). That's probably fine, but the reprioritization code in loadgroup should probably not call SetPriority if GetPriority failed. >Index: uriloader/base/nsDocLoader.cpp >+NS_IMETHODIMP nsDocLoader::SetPriority(PRInt32 aPriority) >+{ >+ // Set the priority of our loadgroup, and then set the priority >+ // of each child nsDocLoader instance. Should this change kids by the delta, which is what loadgroups do, instead of just overriding them? Could you add a comment to the "loadgroups should be in loadgroups" bug about removing this code when that's fixed? sr=bzbarsky with all that addressed.

> I'm not so happy with this part. Could you ok it with Stuart? > Does this leave it possible to load images at the desired priority for > imagelib users who don't want LOW? > > Doing this in nsContentUtils instead like biesi suggests would make a lot of > sense, if that would work. It seems to me that it should. The problem here is that from the point of view of nsContentUtils the LoadImage call could end up piggy backing on an existing load. If the priority of that load was already decremented by say nsImageFrame (b/c the image happens to be associated with a frame -- I have another patch for this), then nsContentUtils would have to be careful not to override that. However, by having the priority be set by LoadImage itself (or rather NewImageChannel), the default is cleanly applied to channels created by image lib. This still gives consumers the ability to alter the load priority of the image request once they have a handle to the image request (i.e., the imgIRequest returned from LoadImage). So, I figure that it is simpler to just set a default load priority for all images created by image lib. > >Index: modules/libpr0n/src/imgRequestProxy.cpp > >+NS_IMETHODIMP imgRequestProxy::GetPriority(PRInt32 *priority) > >+{ > >+ NS_ENSURE_STATE(mOwner); > >+ *priority = mOwner->Priority(); > >+ return NS_OK; > > So... this can leave priority uninitialized (if mOwner is null). that's fine. the method is also returning an exception in that case, so the value of the out param upon return is undefined anyways. > That's probably fine, but the reprioritization code in loadgroup should > probably not call SetPriority if GetPriority failed. good catch. i'll fix that. > >Index: uriloader/base/nsDocLoader.cpp > >+NS_IMETHODIMP nsDocLoader::SetPriority(PRInt32 aPriority) > >+{ > >+ // Set the priority of our loadgroup, and then set the priority > >+ // of each child nsDocLoader instance. > > Should this change kids by the delta, which is what loadgroups do, instead of > just overriding them? hmm... sure. that's a good idea, though i'm not sure that we have any use cases where it would matter. > Could you add a comment to the "loadgroups should be in loadgroups" bug about > removing this code when that's fixed? yeah, good idea.

> If the priority of that load was already decremented by say nsImageFrame The only thing imageframe loads are the placeholder images. The load of the "main" image is done through nsContentUtils....

bz, let me clarify. i added code in nsImageFrame::Init that decrements the priority of the imgrequest associated with the imageloadingcontent passed into the imageframe's init method. it seemed to me that this was the link between imageloadingcontent and the frame tree. that seemed to cause those images to be loaded with higher priority. my testcase was to create a bunch of <img> elements via document.createElement and not actually stick them in the document until some user action. it had the effect of giving higher priority to images that were in the DOM, which seems ideal given that people like to prefetch images for menus and such this way. please let me know if there is a better way to figure out when an image request has been associated with a layout frame.

over IRC bz made the excellent point that we have to be careful not to decrement the priority of the same image request more than once just because it happens to be associated with multiple image frames.

I see what you mean about the interaction of nsContentUtils and nsImageFrame. Maybe nsContentUtils should only mess with the priority if the priority is 0? But I guess with loadgroup prioritization that leads to problems... hmm....

I suppose I could compare the priority of the loadgroup to the image request to determine if the image request should be given a boost in priority. That's making the image frame know a lot about this system, but that's the best solution I've come up with so far.

Yeah, that may be the way to go... Note that we do still want to distinguish between LOAD_BACKGROUND loads and other image loads in nsContentUtils. So maybe all the logic of this sort should live in nsContentUtils methods.

Ok, after talking with stuart on IRC, I decided on a new plan: 1) Make imgRequestProxy::SetPriority only affect the priority of the underlying imgRequest if it is the first imgRequestProxy. 2) Make imgLoader not set any default priority for channels that it creates. 3) Make nsContentUtils decrease priority for any imgIRequest instances created by a call to imgILoader::LoadImage. 4) Make nsImageFrame increase priority for any imgIRequest associated with a frame. This isn't perfect, but I think it'd be a good first solution to this problem. Eventually we might want to make imgIRequest return some sort of weight based on the number of imgRequestProxy instances that can be incorporated into the load priority of the request. Or, maybe imagelib can manage this internally. Also, I'm leaning toward nsISupportsPriority because I think it expresses this interface best.

Sounds good, as long as the resulting "image associated with frame" priority is lower than the priority for toplevel documents. By the way, do we want (in the HTTP code) to slowly increase the priorities of things that have been sitting in the queue for a long time so that they're guaranteed to load in a finite amount of time even if other things are being added to the queue at intervals?

One other thing. The priority that nsContentUtils sets should depend on whether the LOAD_BACKGROUND flag is set, imo.

> Sounds good, as long as the resulting "image associated with frame" priority > is lower than the priority for toplevel documents. Agreed. > By the way, do we want (in the HTTP code) to slowly increase the priorities of > things that have been sitting in the queue for a long time so that they're > guaranteed to load in a finite amount of time even if other things are being > added to the queue at intervals? That sounds like a good idea. I'll look into that as a follow-up. > The priority that nsContentUtils sets should depend on whether > the LOAD_BACKGROUND flag is set, imo. Yes. I'll penalize those by maybe an additional 1 or 2 points perhaps.

OK, I forgot something important: It's wrong for nsContentUtils::LoadImage to try to set the priority after it has created the image request. Why? Well, as I mentioned earlier, but then promptly forgot, once AsyncOpen has been called, the request will have been scheduled. That means that it will go into the queue with NORMAL priority. Is this what we want? No, probably not. We want LOW priority. As a result, we have two problems: new image requests may displace higher priority stuff (once we start fetching we don't stop just because the thing became low priority), and we end up eating CPU cycles to reschedule the request. It just makes sense to assign the priority _before_ AsyncOpen is called. So, how do we do that? Two solutions: 1) Add a priority parameter to LoadImage so that we don't have to give image requests a LOW priority by default. 2) Make LoadImage give image requests a LOW priority by default. Adding new parameters to LoadImage is unattractive given the current number of parameters, so I vote for option 2. We can implement 1 if 2 becomes a problem. NOTE: remember that this is only going to apply to HTTP loads until someone implements nsISupportsPriority in some other protocol handler (which seems unlikely).

BTW, callers of imgILoader::LoadImage can always increase the priority of an image request without fear of the "displacement problem" that applies when trying to go the other way. So, again option 1 :)

So wait. We call LoadImage(). Say LoadImage() doesn't set the priority itself. There are two possibilities: 1) There is nothing in the HTTP request queue; the request gets dispatched. 2) There is something in the HTTP request queue; the request gets queued. Now we change the priority. You're right that it won't affect case #1, but in case #2 it will resort accordingly, no? Note that if LoadImage sets the priority itself, we'll still get immediate dispatch in case #1 (since there is nothing in the queue). So the only drawback of setting priority outside LoadImage is the extra time spent resorting the HTTP queue...

you forget that the queue is processed on a background thread: suppose we have a couple items in the queue that have priority 5, and then imagelib inserts an image with priority 0. then we give that image priority 10 immediately following its insertion into the queue. if a connection gets freed up at just about the same time, the image request could go out before the priority 5 items. that seems like a problem to me.

I just didn't realize the queue processing was off the main thread. No forgetting involved. ;)

ok, here's the revised patch. i added a bumpPriority method to nsISupportsPriority to simplify the task of incrementing or decrementing the priority of an object.

Comment on attachment 174742 [details] [diff] [review] v2 patch The new idl is missing... want to attach that too? >Index: uriloader/base/nsDocLoader.cpp >+NS_IMETHODIMP nsDocLoader::GetPriority(PRInt32 *aPriority) >+{ >+ if (mLoadGroup) { >+ nsCOMPtr<nsISupportsPriority> p = do_QueryInterface(mLoadGroup); do_QI is null-safe, so no reason to check mLoadGroup, since we null-check |p|. Similar for the other methods added to this class. sr=bzbarsky with that, though I still want to see the idl.

Sorry about that. Here's the IDL file.

Comment on attachment 174742 [details] [diff] [review] v2 patch netwerk/base/src/nsLoadGroup.cpp + mPriority += aDelta; + if (aDelta != 0) + PL_DHashTableEnumerate(&mRequests, RescheduleRequests, &aDelta); nit: why not move the += into the if as well? netwerk/protocol/http/public/nsIHttpChannelInternal.idl +[scriptable, uuid(6f225cf9-dfeb-4b9f-8ff5-2a5bf31b73a1)] looks like this is still different from the old IID of this interface :-) netwerk/protocol/http/src/nsHttpChannel.cpp nit: it seems the implementation of Bump/SetPriority would be slightly simpler if BumpPriority called SetPriority and that did the work (no need to call value-mPriority that way) of course this way is more consistent with nsLoadGroup... modules/libpr0n/src/imgLoader.cpp + p->BumpPriority(priority); this is Bump rather than Set because of loadgroups? hm, unrelated thought: nsISupportsPriority.idl does not say what the initial value is. should it? or, should it say explicitly that it is left to the implementation? modules/libpr0n/src/imgRequest.cpp + // only the first proxy is allowed to modify the priority of this image load. hm... given that NewImageChannel now sets the priority, is this still needed? Ah, this is for the image frame's BumpPriority? modules/libpr0n/src/imgRequest.h + // Return the priority of the underlying network request, or return 0 if it nit: maybe PRIORITY_NORMAL rather than 0? uriloader/base/nsDocLoader.cpp + if (mLoadGroup) { + nsCOMPtr<nsISupportsPriority> p = do_QueryInterface(mLoadGroup); nit: do_QI is nullsafe... no need for the if So, it seems that new children of a docloader will not get a priority set on them. maybe that should be fixed. though, putting docloaders in loadgroups would fix this automatically. (hm... I may have mentioned this already somewhere...)

another thought: What about CSS files, scripts, etc? shouldn't they also get a lower priority? Shouldn't link click loads get a higher priority? maybe that's better left for another patch.

I've been thinking about how to prioritize image loads more efficiently, and think it would help if these were a bit further apart: like -200, -100, 0, 100, 200. It would let me give a bit more meaning to the actual image priority and let me increase it more frequently (without having to clamp or put artifical division in0 without worrying about hitting highest very quickly. + const long HIGHEST = -20; + const long HIGH = -10; + const long NORMAL = 0; + const long LOW = 10; + const long LOWEST = 20;

One other thought I had.... If docloader exposes the interface via GetI but not QI, then once we have loadgroups in loadgroups we can just have GetI return the loadgroup. That would avoid the need for "legacy" glue code at that point... Stuart, the problem is that the interface was considered as also a possible interface to "renice", and that clamps values to -20 to 20.... Perhaps that's not an issue? Or perhaps we should say that implementations may scale, with remainders dropped, rather than clamping if the implementation does not support the full long range? Then we could honestly set PRIORITY_HIGHEST to PR_INT32_MAX and such, too.

One other thing. Are the priority setting methods generally allowed to throw? The wording about priority changes being disallowed makes it sound like a specific error will get thrown then...

bz, biesi: thanks for reviewing the patch. lot's of good feedback. (In reply to comment #41) > another thought: What about CSS files, scripts, etc? shouldn't they also get a > lower priority? > > Shouldn't link click loads get a higher priority? > > maybe that's better left for another patch. right, i want to give link clicks slightly higher priority, but i'm saving that for another patch. the current patch still helps because inline images are given lower priority. (In reply to comment #42) > I've been thinking about how to prioritize image loads more efficiently, and > think it would help if these were a bit further apart: like -200, -100, 0, > 100, 200. It would let me give a bit more meaning to the actual image > priority and let me increase it more frequently (without having to clamp or > put artifical division in0 without worrying about hitting highest very > quickly. why do you need so much granularity? i think we could get away with classifying images into maybe 3 priority buckets at most, and that would be just fine. (In reply to comment #43) > One other thought I had.... If docloader exposes the interface via GetI but > not QI, then once we have loadgroups in loadgroups we can just have GetI > return the loadgroup. That would avoid the need for "legacy" glue code at > that point... I think this is a non-issue. We'll have maybe one consumer of this interface via docloader, and that'll be the tabbrowser code. We can change any of this at any time. We can make the tabbrowser code access nsISupportsPriority via GI, but why bother when we control both the consumer and the implementation?

(In reply to comment #40) > modules/libpr0n/src/imgLoader.cpp > + p->BumpPriority(priority); > > this is Bump rather than Set because of loadgroups? yes, and because the channel may impose some other default priority for whatever reason. it's better to code offsets than absolutes, i think. > hm, unrelated thought: nsISupportsPriority.idl does not say what the initial > value is. should it? or, should it say explicitly that it is left to the > implementation? hmm... perhaps it should say that the initial value is implementation specific. clearly, imgRequest sets its initial priority value in an interesting way. > modules/libpr0n/src/imgRequest.cpp > + // only the first proxy is allowed to modify the priority of this image > load. > > hm... given that NewImageChannel now sets the priority, is this still needed? > > Ah, this is for the image frame's BumpPriority? YES :) if the same image appears 100 times on the page, we don't need to give it a 100x priority boost. though pavlov made the point that it should probably get some priority boost, we probably still want it to take second-seat to link clicks and such. > So, it seems that new children of a docloader will not get a priority set on > them. maybe that should be fixed. though, putting docloaders in loadgroups > would fix this automatically. > (hm... I may have mentioned this already somewhere...) Yeah, i'm not going to try to fix this just yet. I want to wait to deal with this issue when I start implementing the code in tabbrowser.

One other thing: I think it is not important to be precise in how Set/BumpPriority may fail. I want this interface to be useful in a variety of applications, and for this application at least, there's no point to reporting errors when Set/BumpPriority cannot do what they are asked to do. For example, if HTTP has already scheduled the request for the highest priority, BumpPriority(-1) would be a no-op. Should we throw an exception in that case? Probably not. I think this rule applies fairly well to all uses that I can imagine. Priority values are advisory. The implementation can choose to do whatever it likes with the given information. Sound fair enough?

Sounds fine (re: comment 47); I think we should just clarify the api to say that implementations should not throw exceptions when priorities are changed. The current API comment makes it sound like an ok thing to do if you feel like it...

landed revised patch on the trunk, marking fixed. i'll deal with tabbrowser and such in a new bug.

> Sounds fine (re: comment 47); I think we should just clarify the api to say that > implementations should not throw exceptions when priorities are changed. The > current API comment makes it sound like an ok thing to do if you feel like it... But exceptions are always possible when it comes to XPCOM methods. Anyways, I went the other way and said that implementations may throw or not throw.

Exceptions are possible, yes. The question is whether the particular case of "changing the priority of an object may be disallowed" is allowed (or forced) to throw, and if so whether we want to define what it should throw. I see that you've clarified that it's allowed to throw. I'd prefer we clarify that it must throw, and which exact exception it will throw (NS_ERROR_NOT_AVAILABLE?).

I think that's overkill. I don't know of any use cases for that exception. None of the implementations throw, and none of the consumers check for exceptions. That doesn't mean that the interface couldn't be used that (i.e., if in prioritizing threads instead of network requests it makes sense for there to be an exception thrown, then we can invent such an exception in the context of threads).

It's interesting to note that this patch had no impact on our Tp tests.

Per request from shaver and brendan over IRC, I changed BumpPriority to AdjustPriority.

Is this being actively exploited yet? I'm not seeing any particularly obvious change/improvement in prioritisation on a current trunk build.