Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Correctly write Long.toBinaryString as an unsigned value #9769

Merged
merged 1 commit into from
Mar 2, 2023

Conversation

niloc132
Copy link
Contributor

A previous fix to negative longs incorrectly allowed some positive longs
that would be represented as negative ints to be printed as negative
binary values, instead of as unsigned values. This patch fixes that case
and adds tests to confirm that these edge cases are well represented.
This bug was introduced by 7616ceb.

The binary, hex, and octal tests all fail before this change, the
toString test was just for completeness, but wasn't actually broken.

Additionally, getHighBits could be expensive, and a range check against
two constants (that easily fit in a "small long") should be cheaper at
runtime. This is not required for the fix, and I will validate this
performance assumption.

Fixes #9764

copybara-service bot pushed a commit to google/j2cl that referenced this pull request Aug 1, 2022
A patch made to GWT last year introduced a bug in turning Longs to
strings - while the same patch was made to J2CL, it did not have an
issue with signs, so these tests should pass as-is. With that said,
there was no test coverage for values that are positive as longs, but
negative when treated as an int, with all high bits set.

See gwtproject/gwt#9769

Closes #155

PiperOrigin-RevId: 464563372
@niloc132
Copy link
Contributor Author

niloc132 commented Aug 4, 2022

I made a quick performance test of the range check vs high bit check, and found that while chrome saw small performance improvements (or at worst saw the same performance most of the time), firefox saw significantly worse performance by making this change, for longs that don't fit in int32. The losses by firefox seemed (from a small sample size) to be worse than the gains seen in chrome, so at least for now I'm voting for keeping the status quo.

In this sample GWT2 app, v1 is the current check that is done, and v2 is the attempted improvement. My theory was that it was cheaper to compare against constants rather than shift, even for large numbers, but that seems to not universally be true.

public class AppEntryPoint implements EntryPoint {
    static final int COUNT = 1_000_000;
    @Override
    public void onModuleLoad() {
        // small positive numbers
        makeTestButton("small positive", 1, Integer.MAX_VALUE);
        // medium positive numbers
        makeTestButton("medium positive", Integer.MAX_VALUE, 1L << 53);
        // large positive numbers
        makeTestButton("large positive", 1L << 55, Long.MAX_VALUE);
        
        // small negative numbers
        makeTestButton("small negative", Integer.MIN_VALUE, -1);
        // medium negative numbers
        makeTestButton("medium negative", -1L << 53, Integer.MIN_VALUE);
        // large negative numbers
        makeTestButton("large negative", Long.MIN_VALUE, -1L << 55);
    }

    private void makeTestButton(String buttonName, long min, long max) {
        Element button = document.createElement("button");
        button.append(buttonName);

        button.onclick = e -> {
            // generate the numbers
            long[] numbers = generate(min, max, COUNT);
            boolean[] results = new boolean[COUNT];

            double[] perf = new double[2];

            Promise.resolve((IThenable<Object>) null)
                    .then(ignore -> {
                        double start = performance.now();
                        for (int i = 0; i < COUNT; i++) {
                            results[i] = fitsInInt32v1(numbers[i]);
                        }
                        perf[0] = performance.now() - start;
                        return null;
                    })
                    .then(ignore -> {
                        double start = performance.now();
                        for (int i = 0; i < COUNT; i++) {
                            results[i] = fitsInInt32v2(numbers[i]);
                        }
                        perf[1] = performance.now() - start;
                        return null;
                    }).then(ignore -> {
                        document.body.append(buttonName, "v1: ", perf[0] + "ms ", "v2:", perf[1] + "ms ", Arrays.toString(results).length() + "");
                        document.body.append(document.createElement("br"));
                        return null;
                    });
            return null;
        };

        document.body.append(button);
    }

    private long[] generate(long min, long max, int count) {
        long[] result = new long[count];
        Random random = new Random();
        for (int i = 0; i < count; i++) {
            result[i] = min + (random.nextLong() % (max - min));
        }
        return result;
    }

    public static boolean fitsInInt32v1(long value) {
        return getHighBits(value) == 0;
    }

    public static int getHighBits(long value) {
        return (int) (value >>> 32);
    }

    public static boolean fitsInInt32v2(long value) {
        // test if no high bits are set, this is the largest int that can be
        // represented with no high bits set
        long negativeOneIntAsLongBits = (1L << 32) - 1;//in int, this is -1

        // verify that no high bits are set by comparing only using "small" longs
        return 0 <= value && value <= negativeOneIntAsLongBits;
    }
}

I'll roll back that attempted improvement, and mark this pull request as ready for review.

A previous fix to negative longs incorrectly allowed some positive longs
that would be represented as negative ints to be printed as negative
binary values, instead of as unsigned values. This patch fixes that case
and adds tests to confirm that these edge cases are well represented.
This bug was introduced by 7616ceb.

The binary, hex, and octal tests all fail before this change, the
toString test was just for completeness, but wasn't actually broken.

Fixes gwtproject#9764
@niloc132 niloc132 force-pushed the 9764-negative-ints-as-longs branch from 90623ea to 916d48d Compare August 5, 2022 14:36
@niloc132 niloc132 marked this pull request as ready for review August 5, 2022 14:43
@niloc132 niloc132 merged commit 7c96744 into gwtproject:main Mar 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Long.toBinaryString is broken in GWT 2.10.0
2 participants