Closed Bug 314890 Opened 19 years ago Closed 19 years ago

Javascript string == compare should test for same string object

Categories

(Core :: JavaScript Engine, enhancement, P4)

Product:
Core
Component:
JavaScript Engine
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.9alpha1

People

(Reporter: jeff, Assigned: mrbkap)

Details

Attachments

(1 file)

Fix
868 bytes, patch
mrbkap
: review+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b5) Gecko/20051006 Firefox/1.4.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b5) Gecko/20051006 Firefox/1.4.1

In the Javascript interpreter, testing string1 == string2 should first
check if the two string objects are the same.  Instead, it seems that
Javascript checks every character of both strings, which is unnecessary
if the two strings happen to be the same string object.


Reproducible: Always

Steps to Reproduce:
1. Create an HTML file with the following:
<html>
<head>
<script type="text/javascript">
var longString = "x";
for (i = 0; i < 20; ++i)
  longString = longString + longString;
alert("created string of length " + longString.length +
      ". If string == tests for same objects, we should count " +
      "an 'instant' 1000 comparisons.");

var longString2 = longString
var count = 0;
for(i = 0; i < 1000; ++i) {
  if (longString == longString2)
    ++count;
}
alert("count = " + count);
</script>
</head>
</html>

2. Open the HTML file in Firefox.
3. Measure the time between the first and second messages as we run the loop
of 1000 string compares.

Actual Results:  
The loop of 1000 string compares takes several seconds, presumably because
it is examining all 1 million characters in each string on each pass.

Expected Results:  
Because we said 
var longString2 = longString
they are the same string object, so 
if (longString == longString2)
could immediately return true and the loop would run almost instantly.

I tried this same Javascript in Internet Explorer 6 and indeed the
loop of 1000 iterations runs almost instantly.
->Core:JS
Assignee: nobody → general
Component: General → JavaScript Engine
Product: Firefox → Core
QA Contact: general → general
Version: unspecified → 1.8 Branch
If I recall correctly, IE reference counts strings and does copy on write. Duping to bug 120977.

*** This bug has been marked as a duplicate of 120977 ***
Severity: normal → enhancement
Status: UNCONFIRMED → RESOLVED
Closed: 19 years ago
Resolution: --- → DUPLICATE
Not a duplicate.  Whether strings are ref-counted doesn't matter.  We could do as the reporter wants, and compare string references before string characters.  This is cheap enough that it is probably worth the overhead, even though in the real world (not in the benchmark in comment 0), JS compares different string references (whether they have the same characters) most of the time.

/be
Status: RESOLVED → UNCONFIRMED
Resolution: DUPLICATE → ---
Could be a general jsval comparison for the equality op macro (before the ltmp == rtmp tag test), or a str1 == str2 test in js_CompareStrings.  Thoughts?

/be
Status: UNCONFIRMED → NEW
Ever confirmed: true
(In reply to comment #4)
> Could be a general jsval comparison for the equality op macro (before the ltmp
> == rtmp tag test)...

Make that after the ltmp == rtmp test, in the then block governed by that test.  If that test is JS_LIKELY (it should be marked that way), then the lval == rval test immediately after it in the then block should be JS_UNLIKELY.

If all the code is small enough to pipeline, no worries.  It would be bad to put the lval == rval test first in an if/else chain, have the compiler assume it was likely true, and fail to fetch stuff for the more common cases.

I claim it is more common by orders of magnitude to compare different strings (whether they have the same chars) to each other.

/be
Attached patch Fix — — Splinter Review 
This has r=brendan.
Assignee: general → mrbkap
Status: NEW → ASSIGNED
Attachment #201786 - Flags: review+
Checked in.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago → 19 years ago
OS: Windows XP → All
Priority: -- → P4
Hardware: PC → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9alpha
Version: 1.8 Branch → Trunk
Checking in regress-314890.js;
/cvsroot/mozilla/js/tests/js1_5/String/regress-314890.js,v  <--  regress-314890.js
initial revision: 1.1

verified with today's trunk on winxp.
Status: RESOLVED → VERIFIED
Flags: testcase+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: