Javascript XMLHttpRequest Object's responseText slow access

RESOLVED FIXED in mozilla1.9.3a5

Status

()

Core
DOM
--
minor
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: danguafer, Assigned: bz)

Tracking

(Blocks: 1 bug)

unspecified
mozilla1.9.3a5
x86
Windows Vista
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Reporter)

Description

7 years ago
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.
(Reporter)

Comment 1

7 years ago
Same issue with any interface-to-browser-dependent-implementation's properties, such HTMLAudioElement's currentTime, for example.
Created attachment 437322 [details]
Self-contained testcase per comment 0, with the typos fixed and such
> 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.
Component: General → DOM: Mozilla Extensions
Product: Firefox → Core
QA Contact: general → general
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)....
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.
Assignee: nobody → bzbarsky
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #437360 - Flags: review?(jonas)
Bug 557605 will give another 5x or so speedup.
(Reporter)

Comment 7

7 years ago
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.
(Reporter)

Comment 8

7 years ago
500.000 access seens desnecessary, but each singular access is showing too slow for many applications.
> 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?
(Reporter)

Comment 10

7 years ago
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
> 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!
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 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 :)
Attachment #437360 - Flags: review?(jonas) → review+
> 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?
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.
Blocks: 560077
Pushed http://hg.mozilla.org/mozilla-central/rev/76e705133759
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
Depends on: 562137
Component: DOM: Mozilla Extensions → DOM
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.