dead-claudia / thallium

A simple, extensible JavaScript test framework (work in progress)
18 stars 4 forks source link

buffer code never invoked - and wrong? #34

Closed ghost closed 7 years ago

ghost commented 7 years ago

In your code in the function matchSameProto you check for isBuffer AFTER this check

if (arrayBufferSupport !== ArrayBufferNone) {

Because of this, isBuffer is never invoked, but will still work! But slower performance (ran benchmark). You have to put the isBuffer check before that function.

Further, your isBuffer code seems to be wrong.

if (isBuffer(a)) {
        return matchView(a, b);
    }

// should be
if (isBuffer(a)) {
        return matchView(a.buffer, b.buffer);
    }

After this fixed, you will get this benchmark result:

 x 6,289,942 ops/sec ±2.48% (88 runs sampled)
ghost commented 7 years ago

Two buffers shiuld not be equal either, but they are. Two different instances are never equal.

dead-claudia commented 7 years ago

Buffers are indexed collections. a.buffer and b.buffer in newer Node return the underlying ArrayBuffer, which cannot be indexed normally. Node pre-4 (e.g. io.js) don't even have this property at all, since it's put there by the internal TypedArray constructor.

The reason why it appears so much faster is because arrayBuffer[i] and arrayBuffer.length don't normally exist. Here's what engines figure out from an ArrayBuffer type:

function matchView(a, b) {
    var count = a.length

    if (count !== b.length) { // always false: `undefined === undefined`
        return false
    }

    while (count) {
        count--
        if (!floatIs(a[count], b[count])) return false
        // Inlined:
        // var a1 = a[count]
        // var b1 = b[count]
        // if (!(a1 === b1 || // always true: `undefined === undefined`
        //         a1 !== a1 && b1 !== b1)) {
        //     return false // never taken: `undefined === undefined`
        // }
    }

    return true
}

Engines optimizing for those types are going to do an IC type check, and with that IC, they'll pretty much optimize it to this:

// Use `ArrayBuffer` map for own properties, remove statically known dead
code
function matchView(a, b) {
    if (!isUnchangedArrayBuffer(a) || !isUnchangedArrayBuffer(b)) {
        return recompile()
    }

    var count = a.length
    b.length

    while (count) {
        count--
        a[count]
        b[count]
    }

    return true
}

// Remove unused properties that aren't connected to getters
function matchView(a, b) {
    if (!isUnchangedArrayBuffer(a) || !isUnchangedArrayBuffer(b)) {
        return recompile()
    }

    var count = a.length

    while (count) {
        count--
    }

    return true
}

// Remove unused arithmetic from loops and branches

// Remove unused properties that aren't connected to getters
function matchView(a, b) {
    if (!isUnchangedArrayBuffer(a) || !isUnchangedArrayBuffer(b)) {
        return recompile()
    }

    a.length
    return true
}

// Remove unused properties that aren't connected to getters
function matchView(a, b) {
    if (!isUnchangedArrayBuffer(a) || !isUnchangedArrayBuffer(b)) {
        return recompile()
    }
    return true
}

So in reality, the isUnchangedArrayBuffer function is very cheap to call (and is usually a single bytecode instruction, like CheckMaps in V8), and the rest is independent of a or b. So this is likely to compile to something like this hypothetical pseudo-assembly (likely inlined):

.matchView_inlined
    push rdi ; a, first argument
    call isUnchangedArrayBuffer
    jz .param1
    j .recompile

.matchView_inlined
    pop rsi ; b, second argument
    call isUnchangedArrayBuffer
    jnz .ret

.matchView_recompile
    push matchView_impl
    call recompileAndCall

.matchView_inlined_ret
    movl rax, 0 ; false

That's why that turns out so fast. Engines use ICs to narrow types down speculatively, and use that to reduce the amount of computation required. Removing dead and redundant code in basic cases like these is relatively straightforward once you know types.


Isiah Meadows me@isiahmeadows.com

On Tue, Sep 27, 2016 at 9:57 AM, zubuzon notifications@github.com wrote:

Two buffers shiuld not be equal either, but they are. Two different instances are never equal.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/isiahmeadows/thallium/issues/34#issuecomment-249872660, or mute the thread https://github.com/notifications/unsubscribe-auth/AERrBEidQxXz_26T76VejPTN25gcCHVkks5quSDjgaJpZM4KHqR3 .

ghost commented 7 years ago

If you got a isView check first, isBuffer will never be invoked. Anyway. I'm closing this, it's a question and not an issue.

dead-claudia commented 7 years ago

@zubuzon I'll clarify that isBuffer will be called for Buffers in Node pre-4 and in browser polyfills (e.g. the standard Browserify one) that don't subclass Uint8Array. Anyways, I'll leave it closed.