-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Remove debug logging in production environment #3949
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
// eslint-disable-next-line | ||
if (process.env.NODE_ENV === 'production') { | ||
module.exports = require('./lib.prod'); | ||
} else { | ||
module.exports = require('./lib'); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
// This file is overwritten before publish | ||
export * from '../lib'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
const logMethods = ['log', 'deprecated', 'info', 'group', 'groupCollapsed', 'groupEnd', 'table']; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @tsherif Eventually this should be moved to luma repo's dev-modules There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not move it to probe.gl? After all it is complementary to the probe.gl log class... |
||
|
||
module.exports = function _(opts) { | ||
return { | ||
visitor: { | ||
MemberExpression(path, state) { | ||
const object = path.get('object'); | ||
|
||
if (object.isIdentifier({name: 'log'})) { | ||
const property = path.get('property'); | ||
const methodName = logMethods.find(name => property.isIdentifier({name})); | ||
if (methodName) { | ||
// Uncomment to debug | ||
// console.log(`${state.file.opts.filename}: log.${methodName} removed`); | ||
path.parentPath.parentPath.remove(); | ||
} | ||
} | ||
} | ||
} | ||
}; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The conventional wisdowm is that
require
kills tree shaking as that works on static analysis ofexport
/import
...Recommend validating that this actually leads to smaller apps. My fear is that this will trigger unconditional bundling of
lib.prod
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
core/lib
is almost entirely dependencies of theDeck
class (with the exception ofLayerExtension
which is super light).This change shaves about 1K off the minified bundle. However, the main concern of including the logging functions is performance, because even when
log.log
returnsnoOp
we spend CPU cycles building the dynamic message strings.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are saying these should always be bundled anyway? Fine, though
// 'require' defeats tree shaking but we always bundle these symbols anyway
)..Got it. FWIW, to handle this, probe.gl has always allowed for message strings to be supplied as functions that only gets evaluated when the logging actually is enabled, e.g instead of:
In theory, this still mints a function (which should be way faster than building a string, but possibly not zero impact, depends on how the JS engine handles this).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran some benchmark tests, it's actually 2-3x more expensive to create those functions inline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for checking and it only goes to prove that actual profiling beats assumptions every time. (Perhaps it is the captured variable that makes it expensive... but that doesn't matter).
In light of this, I'd consider modifying the various probe.gl log calls to take multiple string segment as parameters and then concat them inside the log functions (only when logging actually happens). That should create no small objects and simple parameter passing is typically extremely fast in modern JS engines.
I opened an issue in probe.gl: uber-web/probe.gl#108.
@Pessimistress Happy to help out with this one if this would be considered helpful to the ongoing optimization push.