Last Comment Bug 552573 - Javascript XMLHttpRequest Object's responseText slow access
: Javascript XMLHttpRequest Object's responseText slow access
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: x86 Windows Vista
: -- minor (vote)
: mozilla1.9.3a5
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
Depends on: 562137
Blocks: 560077
  Show dependency treegraph
 
Reported: 2010-03-15 17:45 PDT by danguafer
Modified: 2013-04-04 13:53 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Self-contained testcase per comment 0, with the typos fixed and such (9.21 KB, text/html)
2010-04-06 09:50 PDT, Boris Zbarsky [:bz]
no flags Details
Patch to cache the Unicode (6.44 KB, patch)
2010-04-06 12:05 PDT, Boris Zbarsky [:bz]
jonas: review+
Details | Diff | Splinter Review
Test for HTMLAudioElement's currentTime reading speed. (376 bytes, text/html)
2010-04-06 16:15 PDT, danguafer
no flags Details

Description danguafer 2010-03-15 17:45:00 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; pt-BR; rv:1.9.2) Gecko/20100115 Firefox/3.6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; pt-BR; rv:1.9.2) Gecko/20100115 Firefox/3.6

It's a performance issue. When accessing the data directly through the responseText property, it's too slow. But when the data is refferenced (or copied, I don't know how it works internally) by another var, its access is at a normal speed.

Reproducible: Always

Steps to Reproduce:
// Self-explained code

function readyStateChangeSlow() {
  if (this.readyState == 4)
    for (var i=0; i < this.responseText.length; i++)
      var whatever = this.reponseText.length;
}

function readyStateChangeNormal() {
  if (this.readyState == 4) {
    var responseText = this.responseText;
    for (var i=0; i < responseText.length; i++)
      var whatever = responseText.length;
  }
}
Actual Results:  
readyStateChangeSlow is slower than readyStateChangeNormal.

Expected Results:  
Same speed.
Comment 1 danguafer 2010-04-01 06:44:19 PDT
Same issue with any interface-to-browser-dependent-implementation's properties, such HTMLAudioElement's currentTime, for example.
Comment 2 Boris Zbarsky [:bz] 2010-04-06 09:50:50 PDT
Created attachment 437322 [details]
Self-contained testcase per comment 0, with the typos fixed and such
Comment 3 Boris Zbarsky [:bz] 2010-04-06 10:05:13 PDT
> such HTMLAudioElement's currentTime, for example.

Sounds like a separate issue.  Please file a bug with a testcase?

The main problem with responseText is that we don't actually store the responseText.  We store the bytes of the response, which are then converted to the unicode responseText string as needed.  The assumption seems to be that consumers will get the responseText and then work with it, instead of getting it over and over again.

This could be changed to cache the unicode responseText, I guess.  Your testcase would still be a lot slower for the responseText over and over case (which is the case in all browsers), since there's just a lot more work to be done in that case.  Getting the length of a JS string is literally a single machine instruction in jit-compiled code, while getting the responseText involves locating the right JS object to get the property from, locating the corresponding C++ object, calling the right C++ function, etc.  Easily orders of magnitude slower.
Comment 4 Boris Zbarsky [:bz] 2010-04-06 10:06:24 PDT
Oh, and once you call the C++ function you have to convert the resulting return value into a JS string (which involves allocating new JS strings on every single responseText get)....
Comment 5 Boris Zbarsky [:bz] 2010-04-06 12:05:02 PDT
Created attachment 437360 [details] [diff] [review]
Patch to cache the Unicode

Speeds up the attached testcase by 20x or so; exact speedup will depend on the text length, of course.
Comment 6 Boris Zbarsky [:bz] 2010-04-06 12:28:28 PDT
Bug 557605 will give another 5x or so speedup.
Comment 7 danguafer 2010-04-06 16:15:02 PDT
Created attachment 437442 [details]
Test for HTMLAudioElement's currentTime reading speed.

Consider testing it in Google Chrome. Its access speed is like accessing any other regular variable/property. I think this behavior should be the expected one.
Comment 8 danguafer 2010-04-06 16:28:55 PDT
500.000 access seens desnecessary, but each singular access is showing too slow for many applications.
Comment 9 Boris Zbarsky [:bz] 2010-04-07 06:41:27 PDT
> Test for HTMLAudioElement's currentTime reading speed.

That has nothing to do with this bug.  Please file a separate bug like I asked you to in comment 3?
Comment 10 danguafer 2010-04-07 12:05:58 PDT
Sorry. Should we consider it fixed?

Here is the new filed bug, generalizing the performance issue:
https://bugzilla.mozilla.org/show_bug.cgi?id=557870
Comment 11 Boris Zbarsky [:bz] 2010-04-07 13:03:48 PDT
> Should we consider it fixed?

Which it?  This bug will fix an obvious issue in the XMLHttpRequest responseText getter.

> Here is the new filed bug

Thank you!
Comment 12 Jonas Sicking (:sicking) PTO Until July 5th 2010-04-14 22:02:26 PDT
For what it's worth, this is still wasteful in the case when the page accesses .responseText while the request is still loading. In that case we'll convert starting from the beginning over and over.

An alternative fix would be to hold on to the charset converter and every time that .responseText is accessed feed any newly received data into it and append the result to mResponseBodyUnicode.
Comment 13 Jonas Sicking (:sicking) PTO Until July 5th 2010-04-14 22:03:12 PDT
Comment on attachment 437360 [details] [diff] [review]
Patch to cache the Unicode

r=me as this works and covers most cases though. But if you want to write a patch for the other approach i'll review that too :)
Comment 14 Boris Zbarsky [:bz] 2010-04-14 22:54:14 PDT
> An alternative fix would be to hold on to the charset converter and every time
> that .responseText is accessed feed any newly received data into it and append
> the result to mResponseBodyUnicode.

That gets into all sorts of complications due to the encoding that we should be using possibly changing as a result of an OnDataAvailable call (e.g. we get the <?xml charset="something"?> stuff).  We could make it work, but it'd be annoying.

I _did_, on the other hand, consider dropping the raw bytes if GetResponseText is called after OnStopRequest.  Worth doing that?
Comment 15 Jonas Sicking (:sicking) PTO Until July 5th 2010-04-15 00:24:13 PDT
I think we need to redo how we do charset detection way. It's bad if .responseText can change as we're receiving more data. We probably shouldn't expose any of it until we've settled for a charset.

But it's sounding like a different bug, so all good here.

Dropping the response body does sound worth doing though. Even though we *may* have to revert that if XHR2 grows a way to get at the raw octet stream.
Comment 16 Boris Zbarsky [:bz] 2010-04-23 11:03:12 PDT
Pushed http://hg.mozilla.org/mozilla-central/rev/76e705133759

Note You need to log in before you can comment on or make changes to this bug.