fix: always evaluate default values (in case it is a method)

[ThijsBroersen]

This PR attempts to fix the same as #1119 but as simple as possible without annotations. Credits to @Andrapyre, author of #1119, who made some magnolia changes in May 2024 to add evaluateDefault. I guess in preparation for usage, among others, in zio-json.
I do not know why his PR in zio-json was not picked up. It went quite stale. Hopefully this PR or the old PR can be picked up because it seems like an important fix.

Closes #1055

Note: I do not claim the bounty as prework was already done but it is unclear if the work was rejected, failed or just never looked at.

[ThijsBroersen]

nice, I did not ran tests for JS and Native
should be fine now

[ThijsBroersen]

Now let's hope nobody is depending on memoized default method values as a feature 🙈

[Andrapyre]

@ThijsBroersen , I was disappointed to see that #1119 never received a review. I don't know why - I asked multiple people multiple times and then left it as is. Now it looks like it would need some rebasing to get it ready for a merge. This PR has the advantage of being less complex, whereas #1119 should be more performant in general, since all non-dynamic defaults will be evaluated at compile time, instead of at runtime. I'm happy to rebase #1119 if a member of the zio-json team confirms that the solution is a go-ahead. Otherwise, best of luck in getting this across the finish line :)

[ThijsBroersen]

@Andrapyre perhaps it would be interesting to see if there is much performance difference. Would evaluating a constant really be a big penalty?
I think the ideal solution would be if magnolia would indicate if the value is a constant or not.

[plokhotnyuk]

I will measure impact of dynamic evaluation of defaults with benchmarks and share report here.

[plokhotnyuk]

Below are results for real-world message samples using JDK-21 on i7-11800H CPU.

Scala 3

Before

Benchmark                         Mode  Cnt        Score        Error  Units
GeoJSONReading.zioJson           thrpt   15     9465.550 ±    165.541  ops/s
GitHubActionsAPIReading.zioJson  thrpt   15   310255.930 ±   4598.464  ops/s
GoogleMapsAPIReading.zioJson     thrpt   15    19164.603 ±    172.197  ops/s
OpenRTBReading.zioJson           thrpt   15   164941.515 ±   1186.926  ops/s
TwitterAPIReading.zioJson        thrpt   15    17456.399 ±    721.673  ops/s

After

Benchmark                         Mode  Cnt        Score        Error  Units
GeoJSONReading.zioJson           thrpt   15     9545.312 ±    192.893  ops/s
GitHubActionsAPIReading.zioJson  thrpt   15   320703.547 ±   3579.725  ops/s
GoogleMapsAPIReading.zioJson     thrpt   15    19379.795 ±    182.351  ops/s
OpenRTBReading.zioJson           thrpt   15   131212.485 ±    816.589  ops/s
TwitterAPIReading.zioJson        thrpt   15    16277.179 ±    472.626  ops/s

Scala 2

Before

Benchmark                         Mode  Cnt       Score      Error  Units
GeoJSONReading.zioJson           thrpt   15   10713.393 ±  179.807  ops/s
GitHubActionsAPIReading.zioJson  thrpt   15  263676.440 ± 8420.895  ops/s
GoogleMapsAPIReading.zioJson     thrpt   15   15986.893 ±  245.732  ops/s
OpenRTBReading.zioJson           thrpt   15  141819.438 ±  402.495  ops/s
TwitterAPIReading.zioJson        thrpt   15   10187.695 ±  742.839  ops/s

After

Benchmark                         Mode  Cnt       Score      Error  Units
GeoJSONReading.zioJson           thrpt   15   10698.177 ±  123.256  ops/s
GitHubActionsAPIReading.zioJson  thrpt   15  255979.354 ± 8965.585  ops/s
GoogleMapsAPIReading.zioJson     thrpt   15   15300.128 ±  350.823  ops/s
OpenRTBReading.zioJson           thrpt   15  134179.618 ±  700.721  ops/s
TwitterAPIReading.zioJson        thrpt   15   10204.254 ±  870.285  ops/s

The most affected (5-25% regression) is OpenRTBReading which is based on nested product types with all fields having own default values defined.

Below is a flame-graph that highlight stack frames (~14.5% CPU cycles) in Magnolia routines that evaluate defaults for the most affected benchmark:

[ThijsBroersen]

@plokhotnyuk could you also show the flame graph for the Before results?

[Andrapyre]

@plokhotnyuk , if it's not too much work, would be interesting to see benchmarks for #1119 and whether it solves this regression.

[ThijsBroersen]

@plokhotnyuk thanks for running the benchmarks