Optimize Raw[f] design

[Hendrikto]

Hi @markuswustenberg,

This is (part of) the optimizations I mentioned in #296, which can now be properly benchmarked, with those fixes.

• By making g.Raw a type, we get significantly improved performance, combined with lower memory usage:

  Before:

  ❯ go test . -bench=BenchmarkRaw -benchmem
  goos: linux
  goarch: amd64
  pkg: maragu.dev/gomponents
  cpu: AMD Ryzen 9 9950X3D 16-Core Processor
  BenchmarkRaw/Raw-32             42365084               29.47 ns/op           48 B/op          2 allocs/op
  BenchmarkRaw/Rawf-32            15052114               76.10 ns/op          112 B/op          4 allocs/op
  PASS
  ok      maragu.dev/gomponents   2.397s
  

  After:

  ❯ go test . -bench=BenchmarkRaw -benchmem
  goos: linux
  goarch: amd64
  pkg: maragu.dev/gomponents
  cpu: AMD Ryzen 9 9950X3D 16-Core Processor
  BenchmarkRaw/raw_element-32             74832258               17.55 ns/op           24 B/op          1 allocs/op
  BenchmarkRawf/formatted_raw_element-32          20046828               60.94 ns/op           64 B/op          3 allocs/op
  PASS
  ok      maragu.dev/gomponents   2.538s
  

• Moreover, we also significantly decrease binary size. On a production app, with this single change, I get:

  ❯ ll -B build/release*
  .rwxr----- 11,823,144 hendrik 25 Mar 15:23 -I build/release-before
  .rwxr----- 11,758,312 hendrik 25 Mar 15:23 -I build/release
  

  And that is with only 58 occurences of g.Raw, which amounts to about 1118 bytes saved per g.Raw instance! Unfortunately, the project is proprietary, so I cannot share the code here, but you should be able to easily verify this.

• All tests still pass, and nothing changes from a library consumer perspective. This is similar to Make Group a type #202, where g.Group was made a type.

• This change also reduces code duplication, conceptually, syntactically, and semantically simplifies the design a lot, making the code easier to read, and helping the compiler.

• It also affords us new possibilities, like writing functions that take parameters of type g.Raw, or defining g.Raw constants.

  const iconX g.Raw = `<svg class="icon icon-x" viewBox="0 0 10 10" stroke="currentColor" xmlns="http://www.w3.org/2000/svg">
  	<path stroke-linecap="round" d="M1 1L9 9M9 1L1 9"/>
  </svg>`

Overall, I think this design is just superior in every way. I was unable to identify any drawbacks, and I only see big advantages.

If you agree with this change, I have further similar improvements, but I wanted to gauge your interest first.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (e05d676) to head (9b86a6a).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##              main      #307   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            6         6           
  Lines          666       664    -2     
=========================================
- Hits           666       664    -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[markuswustenberg]

Oooh, nice. I have a feeling that's one of those obvious-in-hindsight changes. :D I'll have a closer look.

[markuswustenberg]

Technically, this is a breaking change. If anyone has been saving a reference to the Raw func of type func(string) Node, their code will break. In practice, I'm not sure anyone would do that. I think the Group change predates 1.0, but I'd have to check.

[Hendrikto]

How about this then? It preserves the advantages while being fully backwards-compatible, as far as I can see.

Edit: Okay, small correction, it does not preserve all the advantages, because now we cannot use raw as a type, as it is not exported. Defining raw constants is nice… hmm 🤷

[markuswustenberg]

How about this then? It preserves the advantages while being fully backwards-compatible, as far as I can see.

Edit: Okay, small correction, it does not preserve all the advantages, because now we cannot use raw as a type, as it is not exported. Defining raw constants is nice… hmm 🤷

Although I can't think of a use case for when the breaking change would be a problem (someone keeping a func(string) Node reference and switching between Text and Raw at runtime?), that doesn't mean there isn't one, so I'm leaning towards keeping the function signature the same and keep full compatibility.

But I think it's still a nice change, even though the Raw constants aren't possible. It's still possible to keep string constants around and pass those to Raw, I think they could have similar performance characteristics.

And now you can do basically the same change for Text/Textf, right?

[markuswustenberg]

I can merge this after just some tiny fixes. :-)

[Hendrikto]

Although I can't think of a use case for when the breaking change would be a problem (someone keeping a func(string) Node reference and switching between Text and Raw at runtime?), that doesn't mean there isn't one, so I'm leaning towards keeping the function signature the same and keep full compatibility.

Yeah, if you want to preserve full compatibility, I fully understand. Something for a potential v2.

But I think it's still a nice change, even though the Raw constants aren't possible. It's still possible to keep string constants around and pass those to Raw, I think they could have similar performance characteristics.

Yes, this version still preserves all other advantages, and still is a big improvement.

And now you can do basically the same change for Text/Textf, right?

Yes, this is what I was talking about above, but it depended on how you wanted to handle this issue, and you indeed brought up the backwards-compatibility issue I did not consider.

[Hendrikto]

Sorry, I mis-clicked. I did not mean to close the PR.

[Hendrikto]

And now you can do basically the same change for Text/Textf, right?

https://github.com/Hendrikto/gomponents/tree/optimize-text

Should I include this in this PR, or open another one?

[markuswustenberg]

Yeah, if you want to preserve full compatibility, I fully understand. Something for a potential v2.

Yes, although I'm hoping I will never have to make a v2. :D

And now you can do basically the same change for Text/Textf, right?

https://github.com/Hendrikto/gomponents/tree/optimize-text

Should I include this in this PR, or open another one?

Nice! I'd prefer a separate PR.

[markuswustenberg]

There must be one line somewhere that's not covered by tests. :D

[Hendrikto]

It’s the String method, which is not used, as raw is already a string.

[markuswustenberg]

It’s the String method, which is not used, as raw is already a string.

Haha, of course. Could you add a small test for it? I'm oddly proud of the 100% coverage. 😅

[Hendrikto]

raw is not exported, so I cannot test it from the current test file, which uses blackbox testing (package gomponents_test). I think there are two options:

1. Add an internal test file (package gomponents) and test raw directly.
2. Test raw indirectly via Raw.

I think option 1 makes more sense, so that is what I implemented for now.

[markuswustenberg]

Great, thanks!

Sorry for my late reply, I've been on an Easter break and haven't been near my computer much.

I think we're ready to merge this?

[Hendrikto]

I've been on an Easter break

I figured as much, no worries :)

I think we're ready to merge this?

If you do not have any objections, I think this is ready, yes.

[markuswustenberg]

@Hendrikto merged! Thank you again for your work. :-) Want to tackle Text next yourself, or should I? Full credit goes to you either way.

[Hendrikto]

Want to tackle Text next yourself […] ?

Yeah, I already had the branch ready for some time, but it depended on this being merged first, so that is why I did not send it before, but I did now.

Thank you again for your work. :-)

Thanks for making gomponents 😄