perf(src/index): regression caused by parsing source maps individually #287

lojjic · 2018-05-04T22:05:59Z

See issue #286

This approach reverts to not using a separate cache object, but being careful about lazily instantiating the SourceMapConsumer only when we know it is needed.

In my project's build, this took the number of instantiated SourceMapConsumer objects from 1418 down to 21.

…ing/error output

jsf-clabot · 2018-05-04T22:06:01Z

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

Jason Johnston seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.

alexander-akait

Bad solution avg on memory usage increase on ~25%, let use map instead weakmap

lojjic · 2018-05-05T14:24:22Z

If you feel this was a "bad" solution, then you really won't like using a Map. At least this approach gives the opportunity for the SourceMapConsumers to be garbage collected as soon as each results handler finishes; using a Map would give no such opportunity, forcing them all to be kept in memory for the lifetime of the plugin.

I'm not sure exactly how you're measuring "avg on memory usage", or what baseline you're comparing against.

alexander-akait · 2018-05-05T14:41:24Z

@lojjic local tests, in near future i will do pr with benchmark npm command. We need source map only when we have warnings/errors. Right now your solution always try to parse source maps even i don't have warning and error. I.E. increase memory/cpu usage when i don't need this.

alexander-akait · 2018-05-05T14:43:38Z

Just replace WeakMap on Map it is allow to find golden way for all (who has errors/warnings and who has not)

lojjic · 2018-05-05T14:44:46Z

Right now your solution always try to parse source maps

That is not true, check the diff. Line 201 after the change.

alexander-akait · 2018-05-05T14:44:46Z

src/index.js

                warning,
                file,
-                inputSourceMap,
+                sourceMap,


~~Also sourceMap can be undefined here, because warning !== error (and parsing in catch block doesn't execute)~~

~~/cc @lojjic yep, my mistake, but if no error(s) - no source map~~

again mistake, sorry, very tired

I'm not following you. sourceMap can be null, if there is no valid inputSourceMap, but that's expected AFAICT.

Ah, ok! No worries.

alexander-akait

Hm, looks very good, let's add test:

source map is good and we have warning
source map is broken and we have warning
source map is good and we have error
source map is broken and we have error

To ensure we have expected behaviour

alexander-akait · 2018-05-05T14:53:28Z

Also i investigate why benchmark increase memory usage on ~25%

michael-ciniawsky

@lojjic Thx

michael-ciniawsky · 2018-05-05T15:30:09Z

src/index.js

@@ -187,11 +171,15 @@ class UglifyJsPlugin {

            tasks.push(task);
          } catch (error) {
+            let sourceMap = null;


michael-ciniawsky · 2018-05-05T15:30:19Z

src/index.js

@@ -208,14 +196,20 @@ class UglifyJsPlugin {
          const { file, input, inputSourceMap, commentsFile } = tasks[index];
          const { error, map, code, warnings, extractedComments } = data;

+          // Create source map if needed
+          let sourceMap = null;


alexander-akait · 2018-05-14T11:12:45Z

/cc @lojjic friendly ping

lojjic · 2018-05-14T16:19:55Z

Thanks for the bump. I understand the desire to fill in the test coverage for those scenarios you mentioned, if they were previously missing.

I've glanced at the tests and I'd definitely have to come up to speed with how the whole plugin works in context as well as how your tests are structured. Given my current time constraints I wouldn't expect that to happen in the next couple weeks.

alexander-akait · 2018-05-14T16:21:01Z

@lojjic thanks for answer, i will try to find time on writing tests

alexander-akait · 2018-05-31T18:51:53Z

Close in favor #305

Thanks!

Fix performance regression caused by parsing sourcemaps on every warn…

e799c33

…ing/error output

alexander-akait suggested changes May 5, 2018

View reviewed changes

alexander-akait reviewed May 5, 2018

View reviewed changes

alexander-akait suggested changes May 5, 2018

View reviewed changes

michael-ciniawsky changed the title ~~Fix performance regression caused by parsing sourcemaps on every warning/error~~ perf(src/index): regression caused by parsing source maps on every warning/error May 5, 2018

michael-ciniawsky added severity: 2 (regression) priority: 3 (required) pr_action: Discuss semver: Patch type: Perf labels May 5, 2018

michael-ciniawsky added this to the 1.2.6 milestone May 5, 2018

michael-ciniawsky approved these changes May 5, 2018

View reviewed changes

michael-ciniawsky changed the title ~~perf(src/index): regression caused by parsing source maps on every warning/error~~ perf(src/index): regression caused by parsing source maps individually May 5, 2018

alexander-akait removed the pr_action: Discuss label May 14, 2018

alexander-akait closed this May 31, 2018

michael-ciniawsky removed this from the 1.2.8 milestone Aug 9, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf(src/index): regression caused by parsing source maps individually #287

perf(src/index): regression caused by parsing source maps individually #287

lojjic commented May 4, 2018

jsf-clabot commented May 4, 2018 •

edited

Loading

alexander-akait left a comment

lojjic commented May 5, 2018

alexander-akait commented May 5, 2018 •

edited

Loading

alexander-akait commented May 5, 2018

lojjic commented May 5, 2018

alexander-akait May 5, 2018 •

edited

Loading

alexander-akait May 5, 2018 •

edited

Loading

alexander-akait May 5, 2018

lojjic May 5, 2018

lojjic May 5, 2018

alexander-akait left a comment

alexander-akait commented May 5, 2018

michael-ciniawsky left a comment

michael-ciniawsky May 5, 2018

michael-ciniawsky May 5, 2018

alexander-akait commented May 14, 2018

lojjic commented May 14, 2018

alexander-akait commented May 14, 2018

alexander-akait commented May 31, 2018

perf(src/index): regression caused by parsing source maps individually #287

perf(src/index): regression caused by parsing source maps individually #287

Conversation

lojjic commented May 4, 2018

jsf-clabot commented May 4, 2018 • edited Loading

alexander-akait left a comment

Choose a reason for hiding this comment

lojjic commented May 5, 2018

alexander-akait commented May 5, 2018 • edited Loading

alexander-akait commented May 5, 2018

lojjic commented May 5, 2018

alexander-akait May 5, 2018 • edited Loading

Choose a reason for hiding this comment

alexander-akait May 5, 2018 • edited Loading

Choose a reason for hiding this comment

alexander-akait May 5, 2018

Choose a reason for hiding this comment

lojjic May 5, 2018

Choose a reason for hiding this comment

lojjic May 5, 2018

Choose a reason for hiding this comment

alexander-akait left a comment

Choose a reason for hiding this comment

alexander-akait commented May 5, 2018

michael-ciniawsky left a comment

Choose a reason for hiding this comment

michael-ciniawsky May 5, 2018

Choose a reason for hiding this comment

michael-ciniawsky May 5, 2018

Choose a reason for hiding this comment

alexander-akait commented May 14, 2018

lojjic commented May 14, 2018

alexander-akait commented May 14, 2018

alexander-akait commented May 31, 2018

jsf-clabot commented May 4, 2018 •

edited

Loading

alexander-akait commented May 5, 2018 •

edited

Loading

alexander-akait May 5, 2018 •

edited

Loading

alexander-akait May 5, 2018 •

edited

Loading