Tweaks

[aminya]

I optimized the SUSPENSE_REPLACE regex

I also modernized some parts of the code by

• using const/let instead of var/let
• using string interpolation
• using Boolean instead of double not

Similar to solidjs/solid-router#17

I used my eslint-config-atomic to detect and fix these issues, but I didn't commit it to the project as I was not sure if you are interested in using the plugin itself. If you are interested, let me know so I commit it. The config is very minimal.

There were some other issues with the code, but I decided to go with this batch.

[coveralls]

Pull Request Test Coverage Report for Build 959700730

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 13 of 13 (100.0%) changed or added relevant lines in 5 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 90.182%

---

Totals
Change from base Build 954130154:	0.0%
Covered Lines:	1054
Relevant Lines:	1121

---

💛 - Coveralls

[maxmilton]

Isn't using string literals and Boolean a step backwards for performance?

1. In older JS engines string literals are much slower than string concatenation and in modern JS engines string concatenation is still faster when combining few items (although string literals are faster when combining many items). Concat is also a few less characters. Simple benchmark shows around 1% to 8% faster performance with short concat; https://jsben.ch/TvgrQ.
2. With Boolean the runtime performance is close to double logical not operators — Boolean object is about 1% to 2% slower in simple benchmarks https://jsben.ch/7OTCJ. However it's 7 more characters. Double not operator is easy to understand so it's not really a readability issue either.

Neither of these things can be optimised by terser etc.

Granted the overall the performance impact is tiny, however, for a framework like this, shouldn't we be brutal when it comes to performance?

[ryansolid]

Hmm.. I didn't think about Boolean effect on Terser.. I haven't found string literals to have any meaningful impact. That being said almost all of these only affect tests and dev mode. Which is the main reason I merged this since it didn't seem to change anything relevant.

I think the Boolean in createRoot is the only thing that ends up in your end user code.

[maxmilton]

I was playing with this and actually terser does transform Boolean(...) when you use the compress: { unsafe: true } option.

Although I often use that option in my own toy projects, it's kind of dangerous when working on large code bases or when the runtime JS environment is non-standard (I sometimes work on web based apps which run on games consoles, TVs, etc. which can be full of surprises in their JS/CSS stack).

[aminya]

@maxmilton
Your benchmarks don't represent the real-world situation, and you should remember once V8 hits Turbofan, all of these comparisons are meaningless.

Anyway, even running your benchmarks, Boolean is faster 1%

If we want to only look at Ignition interpreter (0 optimizations), this is what v8 generates:

const booled = !!x

         00000393081D2EF9 @   15 : 0b fa             Ldar r0
         00000393081D2EFB @   17 : 54                ToBooleanLogicalNot
         00000393081D2EFC @   18 : 55                LogicalNot
         00000393081D2EFD @   19 : bd                Star6

const booled = Boolean(x)

         00000386081D2EFD @   15 : 21 02 00          LdaGlobal [2], [0]
         00000386081D2F00 @   18 : b7                Star12
         00000386081D2F01 @   19 : 61 ee fa 02       CallUndefinedReceiver1 r12, r0, [2]
         00000386081D2F05 @   23 : bd                Star6

As you see, the second generates only one operation which is CallUndefinedReceiver1, but the previous one generates two operations ToBooleanLogicalNot and LogicalNot.

If your app is suffering from huge size, there are better ways to reduce it than trying to remove some characters here and there. Considering Gzip, Boolean is actually better since it is a sequence of characters that are repeated, and the gzip algorithm can reference its usage all over the file. This might not worth it for !!.

[maxmilton]

Oh, nice! That's exciting because I've always favoured !!.

I ran the benchmark on various OS + browser combos and got mixed results. In most cases Boolean is faster, especially so in Firefox and modern desktop Safari 👍 👍 🙂 In up-to-date Chromium-based browsers it seems they're even (running the benchmark multiple times the winner changers). On older Chromium versions the benchmarking site crashes 😅

Yes, it's an additional op for the double logical not, which was a mystery why it was traditionally faster. I always assumed either double logic not compiled into some fast path or Boolean() done some extra checks under the hood.

Shouldn't the benchmark result account for code executed through Ignition, Sparkplug, and Turbofan? TBH I'm not very knowledgeable about the underlying schematics of when JS or bytecode gets promoted to an optimising compiler... I just assume after a certain number of runs and no deopts but there's probably a lot more depth to it. Not important for this discussion I guess but fascinating anyway.

!! is also a repeated string that can be turned into a single symbol within the gzip algorithms so in theory it's equally compressible, no? In practice, it comes down to the individual code base as to how the gzip algo will branch and symbolise so I wouldn't necessarily say one is better than the other (with my limited understanding of the gzip DEFLATE algorithm). Brotli compression though I have no idea... I'm under the impression brotli on the web uses a custom static dictionary so if that dictionary contains Boolean( it would likely be more optimal.