Bug 43400 – Add priority attribute to XMLHttpRequest

• Status: RESOLVED
• Resolution: WONTFIX
• Component: WebKit Misc.

[James Simonsen]

Add a priority attribute to XMLHttpRequest.

Spec is as discussed in this thread on public-webapps:

http://www.mail-archive.com/[e-mail]/msg08218.html

[James Simonsen]

Created attachment 63294 [details]
Patch

[Anne van Kesteren]

I added a comment here: http://lists.w3.org/Archives/Public/public-webapps/2010JulSep/0338.html

[Alexey Proskuryakov]

Comment on attachment 63294 [details]
Patch

This should at least be behind an ifdef, so that platforms that ignore this flag wouldn't look like they honor it to JS code. Maybe this shouldn't be in WebKit trunk at all, as it's purely an experiment.

+    const char* const names[5] = { "critical", "high", "normal", "low", "lowest" };

Normally, constants are numbers, not strings. See e.g. XMLHttpRequest.readyState.

[James Simonsen]

(In reply to comment #3)
> (From update of attachment 63294 [details])
> This should at least be behind an ifdef, so that platforms that ignore this flag wouldn't look like they honor it to JS code. Maybe this shouldn't be in WebKit trunk at all, as it's purely an experiment.

The proposal (http://www.belshe.com/test/xmlhttprequest.priorities.html) says it's safe to ignore:

"Browsers are not required to support the priority requested by applications, and may ignore it altogether."

I think it's fine that most WebKit implementations ignore it. Chrome will use it soon.

> 
> +    const char* const names[5] = { "critical", "high", "normal", "low", "lowest" };
> 
> Normally, constants are numbers, not strings. See e.g. XMLHttpRequest.readyState.

It's modeled after lineCap and lineJoin in the canvas tag. I think the thinking is that it'd be easier to add to or change the priorities later.

[James Simonsen]

(In reply to comment #2)
> I added a comment here: http://lists.w3.org/Archives/Public/public-webapps/2010JulSep/0338.html

Sorry I missed that. I'll send a new patch in a bit.

[James Simonsen]

Created attachment 63363 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 63363 [details]
Patch

r- for the same reasons as before.

[Tony Gentilcore]

Guarding this behind a USE macro seems like the right thing to do.

Also, it's worth pointing out there is a mozilla patch (but it appears to be stalled):
https://bugzilla.mozilla.org/show_bug.cgi?id=559092

FWIW, the moz patch goes the route of a integral constants instead of strings like ap suggests.

[Mike Belshe]

To clarify the ints vs strings issue:
   - the very first draft (prior to feedback from the mlist) was ints.
   - the mozilla patch was implemented before the feedback and has not been committed (as far as I know)
   - based on mlist feedback we changed to strings

I don't think ints vs strings are critical, but the latest version is what we had achieved consensus around on the mlist.

[Alexey Proskuryakov]

> Guarding this behind a USE macro seems like the right thing to do.

My understanding is that these should be ENABLE macros, not USE ones. I don't have a link to any document describing the differences, unfortunately.

[James Simonsen]

Created attachment 63539 [details]
Patch

[James Simonsen]

I've added the ENABLE macro.

I left the enum as a string as it's defined in the proposal. I'm happy to change it to ints if you'd like, but I think we should change the proposal to match.

For the layout tests, I believe the correct thing to do is to have them expect to fail. Once this feature is enabled in Chrome (in a separate patch), I'll add a different -expected.txt for Chrome where all tests pass. Please let me know if this is wrong.

[Alexey Proskuryakov]

Thanks for adding the ENABLE guards!

+FAIL xhr.priority should be normal (of type string). Was undefined (of type undefined).

I'm not super happy with the addition of priority to xmlhttprequest-default-attributes test. Now we're getting a "FAIL" line in output for something that isn't a failure. This experimental feature may or may not get added to XHR spec, and may or may not be enabled in shipping products at some point, so startling developers with FAIL in common test results isn't great.

[Anne van Kesteren]

FYI, I expect to add this to the XMLHttpRequest Level 2 specification once I revised the original XMLHttpRequest test suite to match today's specification. I got nothing but positive feedback from people about this feature.

[Alexey Proskuryakov]

Oh, interesting. In general, I'm not thrilled by features that will likely be only used by 5-10 super incredibly optimized sites on the Web. If someone is willing to invest such enormous resources in optimizing a site (and manually specifying loading priorities is not easy), they are probably much better served by making a native application instead. For others, this is just an opportunity to make a mistake, and to slow down loading.

[James Simonsen]

Created attachment 63617 [details]
Patch

[James Simonsen]

I reverted the default value layout test and instead moved the priority default value test into the priority-enum layout test.

[Eric Seidel (no email)]

Comment on attachment 63617 [details]
Patch

WebCore/xml/XMLHttpRequest.cpp:401
 +      const char* const names[5] = { "critical", "high", "normal", "low", "lowest" };
Since we have this list...

WebCore/xml/XMLHttpRequest.cpp:412
 +      if (s == "critical") {
why not just use it here.  And use a for loop for the search?  Sure, it's "slow", but this code is not hot.  And will be less code (and less error prone).

Otherwise this looks OK.

[Eric Seidel (no email)]

I agree with AP that this is a bad feature.

I'm not sure we want this in WebKit.  I think this is just going to cause trouble for sites and browsers and not speed up the web.

[Eric Seidel (no email)]

I recommend we close this as WONTFIX.

[Eric Seidel (no email)]

My r- is more about my preference for implementation using a for instead of an if cascade.  A "nit" for sure, but I think it might make the code slightly less error-prone.

I'm ambivalent about this.  I feel very uninformed.  Mike Belshe attempted to inform me some this afternoon.

I don't wish to stand in the way of progress.

Mike explained that this is designed to be an experiment to see if better performance can be delivered to complex applications like Google Maps.  The "priority" level is intended as a hint to the browser, not as a service guarantee.

I wonder if the scheduling could/should be implemented at the WebCore level instead of in the network stack.  Maybe it has to be implemented in the network stack.

How does the "priority" of XHR requests relate to the priority of requests generated from the browser?

Mike mentions that the desire here is to have an experiment which can be run for a few (3-4) months.

[Mike Belshe]

To clarify on some points for Eric:
* The browser implementation is intentionally left open so that we don't lock browsers into a very specific implementation.  Thus, the attribute is a hint, not a mandate.
* We intentionally designed it to be backward compatible with the existing API, and, it can also easily be removed with no negative effect.
* It's not true that this can be implemented completely in JS today.  In JS, you can schedule a lot of your own requests.  But, JS applications cannot schedule their requests with knowledge of other work the browser is doing, cannot understand network properties like if there is a proxy, if it is a high speed link, a slow-speed link, etc.
* Note that we specifically only applied it to XHR, which is already an advanced corner of the web.  It's not exposed onto HTML for a reason - to keep it as an advanced feature.

I do concede that we don't know that this will be a big win.  I'd highly recommend most websites stay far away from this feature for a long time until it is thoroughly understood.  But I also know that we can't research and test it without getting this API plumbed through.

Finally, just to make sure people don't think this is willy-nilly, here is some history.
* Various teams have been asking chrome for this for some time (maybe a year?)
* We put together a proposal about 3 months ago after we had solid improvement data from what the maps team was able to do in JS alone.
* That proposal went over to the W3C, and had no real negative feedback (as Anne noted already)
* Here we are.

I hope this background is useful.

[James Simonsen]

Created attachment 63779 [details]
Patch

[James Simonsen]

(In reply to comment #21)
> I wonder if the scheduling could/should be implemented at the WebCore level instead of in the network stack.  Maybe it has to be implemented in the network stack.

I think it's doable to implement this in WebCore. We'd basically need to simulate the network layer's backlog when several XHRs have been sent from WebCore. I think we'd do this by placing a cap on the number of XHRs sent to the network layer from WebCore and queuing those that exceed the cap based on the description in the proposed spec. I can hack this up in a separate bug.

Are there any further comments on this bug?

[Adam Barth]

Comment on attachment 63779 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=63779&action=prettypatch

Nits below.  I don't have an opinion on whether WebKit wants this feature.

> WebCore/xml/XMLHttpRequest.cpp:176
> +#endif // ENABLE(XMLHTTPREQUEST_PRIORITY)
We don't need the comment here.

> WebCore/xml/XMLHttpRequest.cpp:406
> +void XMLHttpRequest::setPriority(const String& s, ExceptionCode& ec)
Please don't use one-letter variables names.  Perhaps s => priority ?

> WebCore/xml/XMLHttpRequest.cpp:413
> +    for (int i = 0; i < 5; ++i) {
The magic constant 5 seems quite decoupled from the size of the array.  Do we have an arraysize macro in this code?  Failing that, a global constant used in place of each instance of 5 would be preferable.

> WebCore/xml/XMLHttpRequest.cpp:418
> +    }
I'd assert the invariants of m_priority at the end of this function, just to be clear about what's going on.

[James Simonsen]

Created attachment 68286 [details]
Patch

[James Simonsen]

Sorry for the extremely slow turnaround. I've addressed all of your comments.

(In reply to comment #25)
> (From update of attachment 63779 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=63779&action=prettypatch
> > WebCore/xml/XMLHttpRequest.cpp:413
> > +    for (int i = 0; i < 5; ++i) {
> The magic constant 5 seems quite decoupled from the size of the array.  Do we have an arraysize macro in this code?  Failing that, a global constant used in place of each instance of 5 would be preferable.

I used sizeof(array)/sizeof(array[0]) and stored that in a global constant. This seems to be the common way for WebKit code to determine array size.

[Adam Barth]

Comment on attachment 68286 [details]
Patch

These ChangeLogs don't tell me anything interesting.  Why are we making this change?  Is there more that needs to be done to complete this feature?  etc.  Also, this patch doesn't seem to do anything.  Has this feature actually been added to the XMLHttpRequest spec?

[Anne van Kesteren]

It's not. It is probably better implemented using a prefix for now, i.e. webkitPriority.

[Olli Pettay (:smaug)]

What is the state of this bug?
I was thinking to update 
https://bugzilla.mozilla.org/show_bug.cgi?id=559092
but before doing that we should agree what the API should look like.

In my patch I was using consts, you're using string, although
internally those are just converted to consts.
I think we could just allow any
priority between some minimum (0?) and max(100?) and then
it is up to the implementation to use those values as hints.
Having numbers makes it easier to increase and decrease the values.
(Mozilla's implementation does allow changing priority while the
XHR is processing the request. It is then up to the network layer to
handle that change.)

So the API could be for example
XMLHttpRequest {
...
  const unsigned short LOW_PRIORITY = 0;
  const unsigned short HIGH_PRIORITY = 100;

  // when setting the value, if bigger than HIGH_PRIORITY
  // priorityHint is set to HIGH_PRIORITY.
  unsigned short priorityHint; 

}


Also, seems like your setPriority may throw an exception.
The patch for Mozilla doesn't do that.

And as Anne says, .priority should be prefixed.

[Alexey Proskuryakov]

I still think that this would be a misfeature.

[Mike Belshe]

Since @ap replied that he thinks this is a misfeature, I wanted to counter that.

Support for XHR Prioritization:
1) Today browsers don't have a priority for XHR requests and cannot determine which order is best.  Apps can help browsers run faster by hinting at which requests are needed most.

2) The solution is simple, backward-compatible, easy to implement, and advisory.  It gives the browser more information to react intelligently without requiring specific behavior.

3) many websites today are writing their own XHR loaders to load all content in order to have priority based loading.  As they do this, they make it more difficult for the browser to help them going forward, and throw a lot of code into javascript.  These solutions are also sub-optimal, as a single webpage never has as much intelligence about network activity as the browser itself.  Sites known that do this today include Google (maps, search, docs, and others) and Facebook.  There are probably more.

4) as we build new web protocols which support priorities natively, XHR prioritization ensures that apps can leverage the new feature.

5) As Anne pointed out (comment #14), there really hasn't been any negative feedback on this feature from the XHR group.  


Negative comments on XHR Prioritization
1) ap (comment #15) said that he doesn't like features which are "only used by 5-10 super incredibly optimized sites on the Web".  I'd agree, except that those 5-10 websites represent a much larger percentage of web traffic.

2) the rest is syntax nits, and minor implementation notes.

[Chad Austin]

Hi,

For our WebGL application, we stream down hundreds of individual assets to load a scene.  Some of these assets (skeletons, meshes, low-res textures) are far more important than others (high-res textures).  In addition, some objects in a 3D scene are more important than others.  Your own 3D character is more important than others.  The room is more important than props in the room.

To minimize load times, we want to make full use of the customer's pipe while also receiving data in order of decreasing importance.

In our native applications, we have the ability to prioritize network traffic appropriately, but on the web, we don't.  Being able to prioritize XMLHttpRequest would be a large improvement to our customer experience.

Thanks,
Chad

[Anne van Kesteren]

This did not get standardized and at this point XMLHttpRequest won't get new features, just bug fixes.