enable conditionnal loading for polyfill

[krichprollsch]

No description provided.

[karlseguin]

Is the reason this is working is that V8 first checks to see if globalThis["fetch"] exists, which executes this callback. We say "NO, it doesn't" (missing always returns false, so we always return v8.Intercepted.No), but then there's something else (what??) v8 is checking, and at that point, fetch has been defined?

Put differently, i understand why this code works the 2nd time if fetch is called twice. But I don't understand why it works the first time.

[krichprollsch]

It seems v8 first checks for native implementation and then looks at the pure JS value.
So the callback is called even if the property exists in pure JS.

return Intercepted::kYes. If the interceptor does not handle the request it must return Intercepted::kNo and it must not produce side effects.
https://v8.github.io/api/head/namespacev8.html#a35b89945995c331fb2e21d5db4e873a7

When returning "no", v8 continues to search for the value, falling back to pure JS. Since we inject a polyfill, we define the property in pure JS, so it works...

I know this is fragile, if the order change, it won't work anymore. But it was really easier than trying to inject the value manually after loading the polyfill.

[krichprollsch]

I will add a comment, but it's important to set the polyfill done immediately.
Indeed, if your polyfill calls the property during its execution, the missing handler will be called too, resulting to an infinite recursive load...

[karlseguin]

These are going to be called a lot, there could be value in optimizing the string comparison to be a single int equality check. Something like this:

switch (name.len) {
    5 => switch (@as(u40, @bitCast(domain[0..5].*))) {
        asUint(u40, "fetch") => if (!self.done.fetch) {
            self.done.fetch = true;
            return load(fetch_src);
        },
        else => {},
    },
    else => {},
};
return false;


fn asUint(comptime T: type, comptime string: []const u8) T {
    return @bitCast(string[0..string.len].*);
}

[sjorsdonkers]

Not certain for this case, but it seems the optimized code generation is better for the std.mem.eql, see: https://godbolt.org/z/8a6M5vc6d

Unless we have measurements I would prefer simple code for the users and the compiler.

std.mem.eql:

switches based:

[krichprollsch]

I changed the code, do you see a good way to optimize this version?

[karlseguin]

@sjorsdonkers I don't think your godbolt code is right. Your code never calls square; I think the function isn't even included in the output. The fact that "fetch" doesn't appear in the assembly is a pretty good sign that something isn't quite right.

Try with:

pub fn main() void {
    const allocator = std.heap.page_allocator;
    const args = std.process.argsAlloc(allocator) catch return;
    defer std.process.argsFree(allocator, args);

    if (square(args[0])) {
       return;
    }

    if (other_square(args[0])) {
       return;
    }
}    

(Removing the std.debug.print and the error return type makes the generated assembly a little less noisy).

Personally, I find this hard to follow, but the switch seems to only require 1-2 compares (which is what I'd expect), vs the 1-4 for std.mem.eql. In both cases, you only get 1 compare on length inequality, and for std.mem.eql you only get 2 jumps if the ptrs are the same (which they never would be for us). After that, it seems to compare in 32 bit chunks (so 2 extra compares for the 40-bit word "fetch").

[sjorsdonkers]

@karlseguin The code doesn't call square as it is generally better to compare the 2 versions in isolation. So to see the other results the function call in the code should be modified.

I agree, it is hard to follow, it is not obvious which implementation will be faster, also considering we don't know how well branch prediction works in either case or how the surrounding code changes things.
Hence why I would recommend avoiding code complexity/micro-optimizations unless we can measure a difference.

[karlseguin]

These are going to be called a lot, there could be value in optimizing the string comparison to be a single int equality check. Something like this:

switch (name.len) {
    5 => switch (@as(u40, @bitCast(domain[0..5].*))) {
        asUint(u40, "fetch") => if (!self.done.fetch) {
            self.done.fetch = true;
            return load(fetch_src);
        },
        else => {},
    },
    else => {},
};
return false;


fn asUint(comptime T: type, comptime string: []const u8) T {
    return @bitCast(string[0..string.len].*);
}

[karlseguin]

I guess the BrowserContext will need its own loader to track the isolate world polyfills?

[krichprollsch]

Good point!

[krichprollsch]

done ✔️

[krichprollsch]

About our discussion regarding returning "true" after having the polyfill loaded, it doesn't work, I get the following error:

INFO  console : error . . . . . . . . . . . . . . . . . . . . [+0ms]
      args =
        1: fetch is not a function
      stack =
        <anonymous>:26
        <anonymous>:28

It's not clear to me why. makes sense in fact: v8 will not call the pure JS version because we stopped it.
If that caes, we should set info.GetReturnValue().Set() to set the return value (by default the result is set to v8::Undefined.
https://v8.github.io/api/head/namespacev8.html#a35b89945995c331fb2e21d5db4e873a7