thedocruby / resounding

A New Minecraft mod that provides realistic audio physics using parallel wave tracing and an improved physics algorithm.
https://thedocruby.dev/resounding
GNU Lesser General Public License v3.0
85 stars 4 forks source link

(WIP) lazy log messages: Replacing or continuing #105 #107

Closed acodili-jg closed 2 weeks ago

acodili-jg commented 11 months ago

Changes from #105:

Removals

Additions

Changes

Old content - ~~`checkErrors(Supplier)` is renamed to `logErrors(Supplier)`~~ - ~~The rename better describes the method, though it seems that `ALUtils.checkErrors(String)` is based on Minecraft's `ALUtil.checkErrors(String)`~~ - Replaced `checkErrors(Supplier)` - Revert changes to `checkErrors(String)` and wrapped the string concat in a supplier - Rename Cache#generate(Consumer) to Cache#generate(LogBuilder) ([2b7bf56](2b7bf5608874a56b40cc2408db64ad1b8f1c0965)) - Replaced `Consumer` loggers to `LogBuilder`, usually acquired using `Logger.atXxx()` or `Logger.atLevel(Level)`, loggers - Though `LogBuilder` is more similar to `Logger`, it could be more restrictive than `Consumer` (e.g. `message -> { doSomething(message); LOGGER.log(message); }`). - Updated commented code - Some templated logging message lines' parameters are wrapped into a supplier - Replace streamed for-each logging into a multiline message sent in bulk - Benefit is that the stream and the operations are only performed when logging can happen (e.g. matching log level) - Behavioral changes, for a theoretical scenario where the logging level changes mid-iterating the stream: - Previous behavior would partially print the entries of the stream - Current implementation would either print the whole stream or none of it
mikenrafter commented 11 months ago

I'll merge this once I get the cache system built.

mikenrafter commented 11 months ago

How are you addressing: lambdas on the heap and lambda closure?

Do you think performance degradation when logging is worth it? If so, could we get some profiling done to know what kind of hit these closure-accessing lambdas cause?

*not being condescending, genuinely curious to hear your take

acodili-jg commented 11 months ago

These didn't come to my mind before being mentioned. Though most lambda-fications are mostly done to ALUtils (and code calling to its members), I was over thinking about limiting the number of lambda closures. Right now, I'm currently thinking about making an alternative to (not outright replace) ALUtils.checkErrors(Consumer<String>) as I'm aiming to use parameterized message logging to replace most lambdas that are connected to ALUtils.

> I had no access to the internet for two days. I did get to see this PR for a bit on that last day. This message totally didn't take a whole morning to reword.
acodili-jg commented 2 weeks ago

Inactive and outdated. I haven't checked in a while but I think it is better to create a new PR if this is still needed (and maybe start over since these aren't big changes) and close this one for now.