Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate log message function syntax in favor of concatenation of params #108

Open
ibgreen opened this issue Dec 3, 2019 · 2 comments
Open

Comments

@ibgreen
Copy link
Collaborator

ibgreen commented Dec 3, 2019

using inline functions for conditional logging take 2-3x more time than unconditionally building strings.

Looks like the function parameter style does not provide the intended performance benefits, see visgl/deck.gl#3949

Proposals:

  • modify the various log calls to take multiple string segment as parameters and concat them inside the log functions when logging actually happens. That should create no objects and parameter passing is really fast in modern JS engines.
  • consider dropping the function parameter feature from probe if it does not provide expected performance benefits (throw or warn if function parameter is passed).
log.info(() => `releasing ${id}`) // old, expensive inline function with variable capture
log.info(`releasing `, id) // new, fast parameter passing, string concatenation happens only of logging enabled.
@ibgreen ibgreen changed the title using inline functions for conditional logging take 2-3x more time than unconditionally building strings. Deprecate log message function syntax in favor of concatenation of params Dec 5, 2019
@Pessimistress
Copy link
Collaborator

I believe most logging functions already support arbitrary number of arguments: https://github.com/uber-web/probe.gl/blob/3.0-release/modules/core/src/lib/log.js#L424

@ibgreen
Copy link
Collaborator Author

ibgreen commented Dec 5, 2019

Exactly, this would be a small additional change.

My point is that 90% of use cases for dynamic log messages is just to concatenate a variable or two into the message, and that can be handled by instead just passing them as bunch of params, which I would expect to be very fast.

The main change called out by this issue is that we would do string concatenation instead of just passing those params as extra args to the corresponding console.log type function, this is mainly to let us control the way things are output, rather than leave it to the browser's implementation of console.log and friends.

(Though as I am testing it now in the Chrome debugger, it actually already looks pretty good the way the debugger prints those args.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants