Bug 5053 - Need to restore int/long changes to simple_number.h
Summary: Need to restore int/long changes to simple_number.h
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Geoffrey Garen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2005-09-19 10:56 PDT by Geoffrey Garen
Modified: 2005-09-22 13:05 PDT (History)
0 users

See Also:


Attachments
Fix (3.76 KB, patch)
2005-09-20 00:20 PDT, Geoffrey Garen
no flags Details | Formatted Diff | Diff
Fix (3.55 KB, patch)
2005-09-20 00:24 PDT, Geoffrey Garen
mjs: review-
Details | Formatted Diff | Diff
Fix (3.72 KB, patch)
2005-09-22 11:42 PDT, Geoffrey Garen
ggaren: review+
Details | Formatted Diff | Diff
fast/js/integer-extremes.html (1.29 KB, text/html)
2005-09-22 11:43 PDT, Geoffrey Garen
no flags Details
fast/js/integer-extremes-expected.txt (17.05 KB, text/plain)
2005-09-22 11:44 PDT, Geoffrey Garen
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Geoffrey Garen 2005-09-19 10:56:23 PDT
The fix to 5028 was to roll them out. Once all the JavaScriptCore tests are passing again, we should figure 
out what was wrong with them and restore them.
Comment 1 Geoffrey Garen 2005-09-19 13:33:52 PDT
To clarify, the goal of the simple_number.h changes was to allow a SimpleNumber to take advantage of a 
64 bit memory space.
Comment 2 Geoffrey Garen 2005-09-20 00:20:05 PDT
Created attachment 3958 [details]
Fix

This is a substantial reworking of the original code, after talking with
Maciej. It passes all the JavaScriptCore tests (I didn't run the date tests)
and the layout tests. 

I have two outstanding questions about this: 

(1) What about the previous code would not have worked in 64 bits?
(2) Are all these casts necessary?
Comment 3 Geoffrey Garen 2005-09-20 00:24:33 PDT
Created attachment 3959 [details]
Fix

Oops. Previous patch had some garbage in it.
Comment 4 Maciej Stachowiak 2005-09-20 02:45:11 PDT
+            maxValue  = (signMask >> 2) - 1,
+            minValue  = signMask >> 2

Should use tagBits instead of 2 here.

+        static inline long value(const ValueImp *imp) { return ((unsigned long)imp >> tagBits) | (((long)
imp & signMask) ? ~0 : 0); }

This doeslike look right to me - ~0 is -1, which has all bits on, so | with it will always give all ones, i.e. 
-1 in all cases. It seems like this would turn all negative numbers that fit in SimpleNumber into -1. (~0 
<< valueBits) might do.

+        static inline bool fits(int i)                { return !(i > static_cast<long>(maxValue) || i < 
static_cast<long>(minValue)); }

Does minValue really come out negative when casted to long? if the enum is unsigned, then minValue 
will be positive, if it is signed, then it will depend on >> doing an arithmetic shift. I think it might be 
clearer to replace the enum with static const long / static const unsigned long declarations, so it's clear 
which of the constants are supposed to be signed and which are supposed to be unsigned. If minValue 
is coming out positive then this patch is probably preventing SimpleNumber from being used at all, 
thus masking any other problems.

Given these concerns, I suggest making more tests and doing performance testing on the next shot at 
this.
Comment 5 Geoffrey Garen 2005-09-20 09:08:18 PDT
Yeah, that | ~0 business was just a brain fart.

I like the static const idea.

I'll post a new patch with a layout test soon. (Pop quiz: how do you test the number machinery without relying on 
numbers? I think I have an interesting answer...)

(In reply to comment #4)
> +            maxValue  = (signMask >> 2) - 1,
> +            minValue  = signMask >> 2
> 
> Should use tagBits instead of 2 here.
> 
> +        static inline long value(const ValueImp *imp) { return ((unsigned long)imp >> tagBits) | (((long)
> imp & signMask) ? ~0 : 0); }
> 
> This doeslike look right to me - ~0 is -1, which has all bits on, so | with it will always give all ones, i.e. 
> -1 in all cases. It seems like this would turn all negative numbers that fit in SimpleNumber into -1. (~0 
> << valueBits) might do.
> 
> +        static inline bool fits(int i)                { return !(i > static_cast<long>(maxValue) || i < 
> static_cast<long>(minValue)); }
> 
> Does minValue really come out negative when casted to long? if the enum is unsigned, then minValue 
> will be positive, if it is signed, then it will depend on >> doing an arithmetic shift. I think it might be 
> clearer to replace the enum with static const long / static const unsigned long declarations, so it's clear 
> which of the constants are supposed to be signed and which are supposed to be unsigned. If minValue 
> is coming out positive then this patch is probably preventing SimpleNumber from being used at all, 
> thus masking any other problems.
> 
> Given these concerns, I suggest making more tests and doing performance testing on the next shot at 
> this.
> 

Comment 6 Geoffrey Garen 2005-09-22 11:42:28 PDT
Created attachment 4006 [details]
Fix
Comment 7 Geoffrey Garen 2005-09-22 11:42:44 PDT
Comment on attachment 4006 [details]
Fix

Darin and Maciej reviewed this.
Comment 8 Geoffrey Garen 2005-09-22 11:43:33 PDT
Created attachment 4007 [details]
fast/js/integer-extremes.html

Added a layout test.
Comment 9 Geoffrey Garen 2005-09-22 11:44:25 PDT
Created attachment 4008 [details]
fast/js/integer-extremes-expected.txt

Added expected test results.
Comment 10 Geoffrey Garen 2005-09-22 13:05:05 PDT
Performance testing with i-bench shows no regression. I'm landing this.