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

Startup Snapshot #13916

Merged
merged 59 commits into from Mar 16, 2017

Conversation

Projects
None yet
9 participants
@as-cii
Member

as-cii commented Mar 2, 2017

For a long time in Atom we have been wanting to have more control over startup time and, as part of this effort, back in November of last year we created an in-depth issue that illustrated where time was (and is) spent during the initialization of an Atom window. What we realized at the time was that most of the operations happening during startup were redundant and that we could use the information we had at build-time to minimize their cost.

V8 snapshots

V8 snapshots allow Electron applications to execute an arbitrary JavaScript file and output a binary file containing a heap with all the data that's left in memory after running a GC at the end of the provided script.

This suits the startup scenario described above perfectly, because we could use it to eagerly perform work when running script/build and just expose the computed data to Atom when it is opened.

The tricky part of using snapshots is that the code is executed in a bare V8 context, meaning that we have no access to Node/Electron features nor to DOM APIs and we can only run plain JavaScript code without having access to native modules either. So, in order to circumvent this issue, we decided to defer usages of forbidden APIs until runtime so that all the other computation could still happen as part of the snapshot. As a result, we set out to create a tool that automates deferring the usage such features in a way that didn't sacrifice code readability: electron-link.

electron-link

electron-link is node module that takes a JavaScript file as the entry point and a list of modules that need to be required lazily. Then, starting from the main module, it traverses the entire require graph, replacing all the forbidden requires in each file with a function that will be executed later at runtime. The output of electron-link is a file that can be passed to the mksnapshot utility to eventually generate the snapshot blob for use in Electron.

electron-link can also determine whether a module can be snapshotted or not. For instance, the following code can be snapshotted:

const path = require('path')

module.exports = function () {
  return path.join('a', 'b', 'c')
}

And the resulting code would look similar to the following snippet:

let path;
function get_path () {
  return path || path = require('path');
}

module.exports = function () {
  return get_path().join('a', 'b', 'c')
}

Conversely, when trying to transform the following code, electron-link will throw an error because it is using a forbidden module at require-time:

const path = require('path')

module.exports = path.join('a', 'b', 'c')

Improving require time

While this pull request lays the groundwork for using snapshots for all the concerns described in #13253, it only speeds up the time we were previously spending in require calls. The reason for beginning with just this one feature is that it will allow us to verify the feasibility of snapshots in production and to ship new performance improvements incrementally.

The following is a typical chart of Atom startup time:

screen shot 2017-03-02 at 12 02 49

Please, note that these benchmarks profile everything from startup till the end of packages activation. There is other stuff going on after activating packages but it depends on asynchronous I/O and the scheduling of idle callbacks, so it adds noise to the measurements.

On average it takes ~ 0.9s - 1.0s to load a stock Atom window and activate its packages when no editor is open and the tree-view is closed. A lot of it is spent in requiring modules and executing them to create classes or define objects. This is how the same chart looks like after this pull request:

screen shot 2017-03-02 at 12 01 21

You can notice how the calls to require have almost disappeared (some still need to take place for Node core modules) and how the total time has went down to ~ 0.7s - 0.8s improving startup time by ~ 15-20%.

Another scenario worth of comparison is loading an Atom window with the tree-view opened. This is how it looks like on master:

screen shot 2017-03-02 at 12 11 24

You can see it was ~ 100ms - 120ms slower and, in part, this is due to loading the package's modules. With the snapshot, on the other hand, we already paid that cost upfront during script/build and therefore the chart looks almost the same in terms of time (we still incur some overhead due to accessing the disk synchronously in the tree-view, but that's another story):

screen shot 2017-03-02 at 12 12 14

This kind of improvement should apply to all the bundled packages because they are now required during the snapshotting phase.

Caveats

Please, note that this pull request disables the asar bundle. Its introduction was mainly driven by two factors:

  1. Lots of file system calls to stat the files.
  2. Long path issues on Windows.

The former will be automatically addressed by snapshots because most of the fs calls will now happen during script/build, whereas the latter was solved by upgrading to npm 3 back in #11897.

Another caveat is that all the code that is snapshotted won't be inspectable in the DevTools, although stack traces should still be present and provide clear information about the call site where the error was thrown from.

Next steps

Overall, we are really excited about snapshots and we think we can further improve startup time by performing even more work during script/build. This could include eagerly constructing an AtomEnvironment, parsing grammars and snippets, loading and parsing less style sheets, etc.

Also, before merging this there are a couple of additional tasks that need to be addressed:

  • Merge atom/fs-plus#37.
  • Merge atom/node-pathwatcher#119.
  • Clean up shouldExcludeModule and remove unnecessarily excluded modules.
  • Use https://github.com/electron/mksnapshot to generate the snapshot.
  • Copy the generated snapshot to the right location on Mac.
  • Copy the generated snapshot to the right location on Windows.
  • Copy the generated snapshot to the right location on Linux.
  • Ensure source maps location translation in stack traces works correctly.
  • Fix tests.
  • Merge #13940.
  • Merge atom/apm#693.
  • Merge #13254.
  • Test drive!

/cc: @atom/maintainers

as-cii added some commits Nov 11, 2016

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Mar 2, 2017

Contributor

Love the thorough explanation on this PR @as-cii. When this lands on beta we should cut and paste this to the blog. ⚡️

Contributor

nathansobo commented Mar 2, 2017

Love the thorough explanation on this PR @as-cii. When this lands on beta we should cut and paste this to the blog. ⚡️

@as-cii as-cii referenced this pull request Mar 2, 2017

Merged

Stop using asar archives #693

@Haacked

This comment has been minimized.

Show comment
Hide comment
@Haacked

Haacked Mar 2, 2017

Love the thorough explanation on this PR @as-cii. When this lands on beta we should cut and paste this to the blog. ⚡️

Great write-up and can't wait for this to land! And I 100% agree with Nathan. This'll make a great blog post. This may be a great candidate for the githubengineering.com as it talks about techniques applicable beyond Atom and we rarely write about the great engineering we do in Client Apps. Not to mention, given we're hiring, we'll want to promote some of the cool things we do. 😄

Haacked commented Mar 2, 2017

Love the thorough explanation on this PR @as-cii. When this lands on beta we should cut and paste this to the blog. ⚡️

Great write-up and can't wait for this to land! And I 100% agree with Nathan. This'll make a great blog post. This may be a great candidate for the githubengineering.com as it talks about techniques applicable beyond Atom and we rarely write about the great engineering we do in Client Apps. Not to mention, given we're hiring, we'll want to promote some of the cool things we do. 😄

@sturnquest

This comment has been minimized.

Show comment
Hide comment
@sturnquest

sturnquest Mar 2, 2017

💯 to what @Haacked and @nathansobo said. Great write-up and a great example of doing great technical work. Nice work!

sturnquest commented Mar 2, 2017

💯 to what @Haacked and @nathansobo said. Great write-up and a great example of doing great technical work. Nice work!

Don't assign snapshotResult.entryPointDirPath
...because we can use `process.resourcesPath` or
`atom.getLoadSettings()` in packages that need to resolve the absolute
path at runtime.
@as-cii

This comment has been minimized.

Show comment
Hide comment
@as-cii

as-cii Mar 10, 2017

Member

I have been using this for a while now and it looks pretty good on my machine, so I think it's ready for a test drive! 🚗

@ungb @Ben3eeE: given that it potentially affects every feature that we ship with Atom, it would be great if you could perform a smoke test on all the platforms and ensure we are not introducing any regression.

I plan to merge this on Monday morning so that we can have the whole week to fix potential regressions that could slip through QA.

Member

as-cii commented Mar 10, 2017

I have been using this for a while now and it looks pretty good on my machine, so I think it's ready for a test drive! 🚗

@ungb @Ben3eeE: given that it potentially affects every feature that we ship with Atom, it would be great if you could perform a smoke test on all the platforms and ensure we are not introducing any regression.

I plan to merge this on Monday morning so that we can have the whole week to fix potential regressions that could slip through QA.

Speed up FileSystemBlobStore.load by not storing invalidation keys
This was unneeded because we can simply compute the cache key by
concatenating the v8 version and the file's contents.
@Ben3eeE

This comment has been minimized.

Show comment
Hide comment
@Ben3eeE

Ben3eeE Mar 15, 2017

Member

@as-cii I got this error when I built b2983f6. I did not get it when I was testing 7caeb3d.

Atom: 1.17.0-dev-b2983f6 x64
Electron: 1.3.14
OS: Microsoft Windows 7 Professional
Thrown From: pigments package 0.39.0

Stack Trace

Failed to activate the pigments package

At Cannot read property 'length' of undefined

TypeError: Cannot read property 'length' of undefined
    at Function.Module._resolveLookupPaths (module.js:305:23)
    at Module._resolveFilename (module.js:446:31)
    at Module._resolveFilename (~/AppData/Local/atom/app-1.17.0-dev-b2983f6/resources/electron.asar/common/reset-search-paths.js:35:12)
    at Function.get_Module._resolveFilename (<embedded>:7794:58)
    at Module.require (~/AppData/Local/atom/app-1.17.0-dev-b2983f6/resources/app/static/index.js:40:43)
    at require (<embedded>:7155:33)
    at requireCore (/packages/pigments/lib/pigments.coffee:295:7)
    at Object.patchAtom (/packages/pigments/lib/pigments.coffee:297:26)
    at Object.activate (/packages/pigments/lib/pigments.coffee:16:6)
    at Package.module.exports.Package.activateNow (<embedded>:66731:25)
    at <embedded>:66703:38
    at Package.module.exports.Package.measure (<embedded>:66609:21)
    at <embedded>:66696:32
    at Package.module.exports.Package.activate (<embedded>:66693:40)
    at PackageManager.module.exports.PackageManager.activatePackage (<embedded>:19559:40)
    at <embedded>:19540:35
    at Config.module.exports.Config.transactAsync (<embedded>:16818:24)
    at PackageManager.module.exports.PackageManager.activatePackages (<embedded>:19535:25)
    at PackageManager.module.exports.PackageManager.activate (<embedded>:19517:52)
    at <embedded>:1061:34

Commands

     -1:40.6.0 tree-view:show (atom-workspace.workspace.scrollbars-visible-always.theme-atom-material-syntax.theme-one-dark-ui)
  2x -1:32.5.0 application:open-folder (atom-pane.pane.active)

Non-Core Packages

advanced-open-file 0.16.6 
atom-autocomplete-php 0.22.2 
atom-beautify 0.29.17 
atom-clock 0.1.6 
atom-material-syntax 1.0.2 
atom-material-syntax-light 0.4.6 
atom-material-ui 1.3.9 
atom-typescript 10.1.15 
autocomplete-emojis 2.5.0 
busy-signal 1.3.0 
column-select 0.2.0 
duotone-light-syntax 2.1.0 
file-icons 2.0.17 
git-pear 1.2.1 
highlight-selected 0.12.0 
intentions 1.1.2 
jshint 1.8.6 
language-babel 2.56.2 
language-markdown 0.20.0 
language-pegjs 0.5.0 
less-than-slash 0.16.0 
linter 2.1.0 
linter-eslint 8.1.4 
linter-js-standard 3.9.0 
linter-php 1.3.1 
linter-ui-default 1.1.0 
markdown-pdf 1.5.0 
merge-conflicts 1.4.4 
minimap 4.26.8 
minimap-autohide 0.10.1 
minimap-highlight-selected 4.5.0 
minimap-pigments 0.2.2 
nebula-syntax 0.4.4 
nebula-ui 0.6.0 
nvatom 0.11.0 
pigments 0.39.0 
stacktrace 0.0.2 
Sublime-Style-Column-Selection 1.7.4 
svn 0.0.13 
symbol-gen 1.3.1 
tabs-to-spaces 1.0.3 
toggler 0.2.0 
tortoise-svn 0.5.0 
vim-mode-plus 0.84.1 
zentabs 0.8.8 
Member

Ben3eeE commented Mar 15, 2017

@as-cii I got this error when I built b2983f6. I did not get it when I was testing 7caeb3d.

Atom: 1.17.0-dev-b2983f6 x64
Electron: 1.3.14
OS: Microsoft Windows 7 Professional
Thrown From: pigments package 0.39.0

Stack Trace

Failed to activate the pigments package

At Cannot read property 'length' of undefined

TypeError: Cannot read property 'length' of undefined
    at Function.Module._resolveLookupPaths (module.js:305:23)
    at Module._resolveFilename (module.js:446:31)
    at Module._resolveFilename (~/AppData/Local/atom/app-1.17.0-dev-b2983f6/resources/electron.asar/common/reset-search-paths.js:35:12)
    at Function.get_Module._resolveFilename (<embedded>:7794:58)
    at Module.require (~/AppData/Local/atom/app-1.17.0-dev-b2983f6/resources/app/static/index.js:40:43)
    at require (<embedded>:7155:33)
    at requireCore (/packages/pigments/lib/pigments.coffee:295:7)
    at Object.patchAtom (/packages/pigments/lib/pigments.coffee:297:26)
    at Object.activate (/packages/pigments/lib/pigments.coffee:16:6)
    at Package.module.exports.Package.activateNow (<embedded>:66731:25)
    at <embedded>:66703:38
    at Package.module.exports.Package.measure (<embedded>:66609:21)
    at <embedded>:66696:32
    at Package.module.exports.Package.activate (<embedded>:66693:40)
    at PackageManager.module.exports.PackageManager.activatePackage (<embedded>:19559:40)
    at <embedded>:19540:35
    at Config.module.exports.Config.transactAsync (<embedded>:16818:24)
    at PackageManager.module.exports.PackageManager.activatePackages (<embedded>:19535:25)
    at PackageManager.module.exports.PackageManager.activate (<embedded>:19517:52)
    at <embedded>:1061:34

Commands

     -1:40.6.0 tree-view:show (atom-workspace.workspace.scrollbars-visible-always.theme-atom-material-syntax.theme-one-dark-ui)
  2x -1:32.5.0 application:open-folder (atom-pane.pane.active)

Non-Core Packages

advanced-open-file 0.16.6 
atom-autocomplete-php 0.22.2 
atom-beautify 0.29.17 
atom-clock 0.1.6 
atom-material-syntax 1.0.2 
atom-material-syntax-light 0.4.6 
atom-material-ui 1.3.9 
atom-typescript 10.1.15 
autocomplete-emojis 2.5.0 
busy-signal 1.3.0 
column-select 0.2.0 
duotone-light-syntax 2.1.0 
file-icons 2.0.17 
git-pear 1.2.1 
highlight-selected 0.12.0 
intentions 1.1.2 
jshint 1.8.6 
language-babel 2.56.2 
language-markdown 0.20.0 
language-pegjs 0.5.0 
less-than-slash 0.16.0 
linter 2.1.0 
linter-eslint 8.1.4 
linter-js-standard 3.9.0 
linter-php 1.3.1 
linter-ui-default 1.1.0 
markdown-pdf 1.5.0 
merge-conflicts 1.4.4 
minimap 4.26.8 
minimap-autohide 0.10.1 
minimap-highlight-selected 4.5.0 
minimap-pigments 0.2.2 
nebula-syntax 0.4.4 
nebula-ui 0.6.0 
nvatom 0.11.0 
pigments 0.39.0 
stacktrace 0.0.2 
Sublime-Style-Column-Selection 1.7.4 
svn 0.0.13 
symbol-gen 1.3.1 
tabs-to-spaces 1.0.3 
toggler 0.2.0 
tortoise-svn 0.5.0 
vim-mode-plus 0.84.1 
zentabs 0.8.8 
@Ben3eeE

This comment has been minimized.

Show comment
Hide comment
@Ben3eeE

Ben3eeE Mar 15, 2017

Member

@as-cii If I do open folder I see a null/coffee folder. Creating files here reveal some files in the tree-view.

Edit: I can't reproduce this and I was doing things I haven't done before so maybe it is just me misunderstanding things. Let's see if I can figure out why I saw these files or not.

image

Member

Ben3eeE commented Mar 15, 2017

@as-cii If I do open folder I see a null/coffee folder. Creating files here reveal some files in the tree-view.

Edit: I can't reproduce this and I was doing things I haven't done before so maybe it is just me misunderstanding things. Let's see if I can figure out why I saw these files or not.

image

@as-cii

This comment has been minimized.

Show comment
Hide comment
@as-cii

as-cii Mar 15, 2017

Member

Thank you so much for testing this, @Ben3eeE! ❤️ I have submitted a pull request on the pigments package that fixes the issue you encountered; they were monkey patching some native modules assuming to find them in require.cache, which won't be necessarily accurate after this pull request gets merged.

As for the null/coffee folder, it's really weird but I haven't encountered that issue myself after using Open Folder quite extensively over the last two weeks. Given that you reproduced it only once, I feel like we could probably consider it as a red herring for now.

It's worth noting that @Ben3eeE was also able to reproduce a hard crash when booting Atom, so I am going to spawn a Windows VM and investigate the problem. Based on the crash dumps he reported, it looks like the error is coming from pathwatcher and looks very similar to what we supposedly fixed in atom/node-pathwatcher#118. There are probably other race conditions and now that Atom loads faster they might have become more evident.

Member

as-cii commented Mar 15, 2017

Thank you so much for testing this, @Ben3eeE! ❤️ I have submitted a pull request on the pigments package that fixes the issue you encountered; they were monkey patching some native modules assuming to find them in require.cache, which won't be necessarily accurate after this pull request gets merged.

As for the null/coffee folder, it's really weird but I haven't encountered that issue myself after using Open Folder quite extensively over the last two weeks. Given that you reproduced it only once, I feel like we could probably consider it as a red herring for now.

It's worth noting that @Ben3eeE was also able to reproduce a hard crash when booting Atom, so I am going to spawn a Windows VM and investigate the problem. Based on the crash dumps he reported, it looks like the error is coming from pathwatcher and looks very similar to what we supposedly fixed in atom/node-pathwatcher#118. There are probably other race conditions and now that Atom loads faster they might have become more evident.

@Ben3eeE

This comment has been minimized.

Show comment
Hide comment
@Ben3eeE

Ben3eeE Mar 15, 2017

Member

Yeah... I'll see about the null folder. I'm completely clueless about where it came from and how to get it to appear. Maybe it was something that did belong to that project but like why?

I am building latest master now to see if the crashes reproduce there but I likely won't be able to test that build on the same computer until tomorrow. The crashes were much more frequent outside of safe mode but I crashed when launching​ safe mode also.

I'll also test and compare launch speed with master now that I got a feel for this. Most of my Atom restarts are because of installing a different version to test regressions so it's hard to quantify speedup for me.

I did find one or two other regressions when testing this that might be from the jQuery removal. I'll open issues for those and mention @as-cii in them.

Member

Ben3eeE commented Mar 15, 2017

Yeah... I'll see about the null folder. I'm completely clueless about where it came from and how to get it to appear. Maybe it was something that did belong to that project but like why?

I am building latest master now to see if the crashes reproduce there but I likely won't be able to test that build on the same computer until tomorrow. The crashes were much more frequent outside of safe mode but I crashed when launching​ safe mode also.

I'll also test and compare launch speed with master now that I got a feel for this. Most of my Atom restarts are because of installing a different version to test regressions so it's hard to quantify speedup for me.

I did find one or two other regressions when testing this that might be from the jQuery removal. I'll open issues for those and mention @as-cii in them.

@Ben3eeE

This comment has been minimized.

Show comment
Hide comment
@Ben3eeE

Ben3eeE Mar 16, 2017

Member

@as-cii I was able to reproduce the crashes on master so it's not a regression in this PR. It's something that should be looked at but not something that should block this PR. I was unable to reproduce the crashes on my Windows 10 machine. The project that I load here is on a network share so maybe Windows has problems watching paths on a network drive.

As for startup speed I see a 2 second improvement on both computers I tested. Loading the network share folder on my Windows 7 machine took about 8 seconds on master and 6 seconds on this branch. Loading a local folder on my Windows 10 machine took about 6 seconds on master and 4 seconds on this branch. Timed using my handy stopwatch, from clicking the icon to Atom being fully rendered and syntax highlighted.

Member

Ben3eeE commented Mar 16, 2017

@as-cii I was able to reproduce the crashes on master so it's not a regression in this PR. It's something that should be looked at but not something that should block this PR. I was unable to reproduce the crashes on my Windows 10 machine. The project that I load here is on a network share so maybe Windows has problems watching paths on a network drive.

As for startup speed I see a 2 second improvement on both computers I tested. Loading the network share folder on my Windows 7 machine took about 8 seconds on master and 6 seconds on this branch. Loading a local folder on my Windows 10 machine took about 6 seconds on master and 4 seconds on this branch. Timed using my handy stopwatch, from clicking the icon to Atom being fully rendered and syntax highlighted.

@Ben3eeE

This comment has been minimized.

Show comment
Hide comment
@Ben3eeE

Ben3eeE Mar 16, 2017

Member
  1. Open a new document
  2. Add the following text: <div id="navtop">
  3. Change the language to HTML
  4. Place the cursor after the " character
  5. Open autocomplete-plus using autocomplete-plus:activate

Atom: 1.17.0-dev-b2983f6 x64
Electron: 1.3.14
OS: Microsoft Windows 7 Professional
Thrown From: Atom Core

Stack Trace

Uncaught TypeError: Cannot read property 'src' of undefined

At <embedded>:37129

TypeError: Cannot read property 'src' of undefined
    at Object.getAttributeValues (<embedded>:37129:52)
    at Object.getAttributeValueCompletions (<embedded>:37069:27)
    at Object.getSuggestions (<embedded>:36901:27)
    at providers.forEach.provider (<embedded>:37473:58)
    at Array.forEach (native)
    at AutocompleteManager.getSuggestionsFromProviders (<embedded>:37449:21)
    at AutocompleteManager.findSuggestions (<embedded>:37441:23)

Commands

  2x -3:48.5.0 core:backspace (input.hidden-input)
     -3:47.7.0 intentions:highlight (input.hidden-input)
     -3:47.6.0 core:save (input.hidden-input)
  4x -3:44.8.0 core:backspace (input.hidden-input)
     -3:41 intentions:highlight (input.hidden-input)
     -3:40.9.0 core:save (input.hidden-input)
  2x -3:37.4.0 core:backspace (input.hidden-input)
     -3:36.4.0 intentions:highlight (input.hidden-input)
     -3:36.2.0 core:save (input.hidden-input)
 14x -3:33.8.0 core:backspace (input.hidden-input)
     -3:24.6.0 core:delete (input.hidden-input)
     -3:24.3.0 intentions:highlight (input.hidden-input)
  2x -3:24.1.0 editor:select-to-end-of-word (input.hidden-input)
     -3:22 core:delete (input.hidden-input)
     -3:21.7.0 intentions:highlight (input.hidden-input)
     -3:21.4.0 core:save (input.hidden-input)

Non-Core Packages

advanced-open-file 0.16.6 
atom-autocomplete-php 0.22.2 
atom-beautify 0.29.17 
atom-clock 0.1.6 
atom-material-syntax 1.0.2 
atom-material-syntax-light 0.4.6 
atom-material-ui 1.3.9 
atom-typescript 10.1.15 
autocomplete-emojis 2.5.0 
busy-signal 1.3.0 
column-select 0.2.0 
duotone-light-syntax 2.1.0 
file-icons 2.0.17 
git-pear 1.2.1 
highlight-selected 0.12.0 
intentions 1.1.2 
jshint 1.8.6 
language-babel 2.56.2 
language-markdown 0.20.0 
language-pegjs 0.5.0 
less-than-slash 0.16.0 
linter 2.1.0 
linter-eslint 8.1.4 
linter-js-standard 3.9.0 
linter-php 1.3.1 
linter-ui-default 1.2.1 
markdown-pdf 1.5.0 
merge-conflicts 1.4.4 
minimap 4.26.8 
minimap-autohide 0.10.1 
minimap-highlight-selected 4.5.0 
minimap-pigments 0.2.2 
nebula-syntax 0.4.4 
nebula-ui 0.6.0 
nvatom 0.11.0 
pigments 0.39.0 
stacktrace 0.0.2 
Sublime-Style-Column-Selection 1.7.4 
svn 0.0.13 
symbol-gen 1.3.1 
tabs-to-spaces 1.0.3 
toggler 0.2.0 
tortoise-svn 0.5.0 
vim-mode-plus 0.84.1 
zentabs 0.8.8 
Member

Ben3eeE commented Mar 16, 2017

  1. Open a new document
  2. Add the following text: <div id="navtop">
  3. Change the language to HTML
  4. Place the cursor after the " character
  5. Open autocomplete-plus using autocomplete-plus:activate

Atom: 1.17.0-dev-b2983f6 x64
Electron: 1.3.14
OS: Microsoft Windows 7 Professional
Thrown From: Atom Core

Stack Trace

Uncaught TypeError: Cannot read property 'src' of undefined

At <embedded>:37129

TypeError: Cannot read property 'src' of undefined
    at Object.getAttributeValues (<embedded>:37129:52)
    at Object.getAttributeValueCompletions (<embedded>:37069:27)
    at Object.getSuggestions (<embedded>:36901:27)
    at providers.forEach.provider (<embedded>:37473:58)
    at Array.forEach (native)
    at AutocompleteManager.getSuggestionsFromProviders (<embedded>:37449:21)
    at AutocompleteManager.findSuggestions (<embedded>:37441:23)

Commands

  2x -3:48.5.0 core:backspace (input.hidden-input)
     -3:47.7.0 intentions:highlight (input.hidden-input)
     -3:47.6.0 core:save (input.hidden-input)
  4x -3:44.8.0 core:backspace (input.hidden-input)
     -3:41 intentions:highlight (input.hidden-input)
     -3:40.9.0 core:save (input.hidden-input)
  2x -3:37.4.0 core:backspace (input.hidden-input)
     -3:36.4.0 intentions:highlight (input.hidden-input)
     -3:36.2.0 core:save (input.hidden-input)
 14x -3:33.8.0 core:backspace (input.hidden-input)
     -3:24.6.0 core:delete (input.hidden-input)
     -3:24.3.0 intentions:highlight (input.hidden-input)
  2x -3:24.1.0 editor:select-to-end-of-word (input.hidden-input)
     -3:22 core:delete (input.hidden-input)
     -3:21.7.0 intentions:highlight (input.hidden-input)
     -3:21.4.0 core:save (input.hidden-input)

Non-Core Packages

advanced-open-file 0.16.6 
atom-autocomplete-php 0.22.2 
atom-beautify 0.29.17 
atom-clock 0.1.6 
atom-material-syntax 1.0.2 
atom-material-syntax-light 0.4.6 
atom-material-ui 1.3.9 
atom-typescript 10.1.15 
autocomplete-emojis 2.5.0 
busy-signal 1.3.0 
column-select 0.2.0 
duotone-light-syntax 2.1.0 
file-icons 2.0.17 
git-pear 1.2.1 
highlight-selected 0.12.0 
intentions 1.1.2 
jshint 1.8.6 
language-babel 2.56.2 
language-markdown 0.20.0 
language-pegjs 0.5.0 
less-than-slash 0.16.0 
linter 2.1.0 
linter-eslint 8.1.4 
linter-js-standard 3.9.0 
linter-php 1.3.1 
linter-ui-default 1.2.1 
markdown-pdf 1.5.0 
merge-conflicts 1.4.4 
minimap 4.26.8 
minimap-autohide 0.10.1 
minimap-highlight-selected 4.5.0 
minimap-pigments 0.2.2 
nebula-syntax 0.4.4 
nebula-ui 0.6.0 
nvatom 0.11.0 
pigments 0.39.0 
stacktrace 0.0.2 
Sublime-Style-Column-Selection 1.7.4 
svn 0.0.13 
symbol-gen 1.3.1 
tabs-to-spaces 1.0.3 
toggler 0.2.0 
tortoise-svn 0.5.0 
vim-mode-plus 0.84.1 
zentabs 0.8.8 
@as-cii

This comment has been minimized.

Show comment
Hide comment
@as-cii

as-cii Mar 16, 2017

Member

Thanks for the feedback, @Ben3eeE. It's so great to hear the improvement on Windows is so dramatic! I have been working on other improvements that take advantage of the snapshot which should minimize those times even further. As soon as this gets merged, I will open a pull request for those improvements too.

atom/autocomplete-html#53 should fix the error you reported (as well as removing an extra fs call 🐎): I have pushed the fix onto this branch, so I think this is ready for another test drive! 👍

Member

as-cii commented Mar 16, 2017

Thanks for the feedback, @Ben3eeE. It's so great to hear the improvement on Windows is so dramatic! I have been working on other improvements that take advantage of the snapshot which should minimize those times even further. As soon as this gets merged, I will open a pull request for those improvements too.

atom/autocomplete-html#53 should fix the error you reported (as well as removing an extra fs call 🐎): I have pushed the fix onto this branch, so I think this is ready for another test drive! 👍

@Ben3eeE

This comment has been minimized.

Show comment
Hide comment
@Ben3eeE

Ben3eeE Mar 16, 2017

Member

Building now Really great work here @as-cii can't wait for the other improvements ⚡️ 💯

Member

Ben3eeE commented Mar 16, 2017

Building now Really great work here @as-cii can't wait for the other improvements ⚡️ 💯

@Ben3eeE

This comment has been minimized.

Show comment
Hide comment
@Ben3eeE

Ben3eeE Mar 16, 2017

Member

@as-cii Can confirm that the HTML exception is gone. Will rebuild again and test css as well just to be sure.
Edit: autocomplete-css now works 🚢

Member

Ben3eeE commented Mar 16, 2017

@as-cii Can confirm that the HTML exception is gone. Will rebuild again and test css as well just to be sure.
Edit: autocomplete-css now works 🚢

@as-cii

This comment has been minimized.

Show comment
Hide comment
@as-cii

as-cii Mar 16, 2017

Member

Okay, so the failure on AppVeyor seems the same we are observing on master right now, so this pull request shouldn't have affected it.

A huge shoutout goes to @Ben3eeE for testing this out and to @ungb for helping me test packages during the jQuery removal. Also, thank you so much @maxbrunsfeld, @nathansobo and @damieng for helping me out with #13254. ⚡️

I will go ahead and 🚢 this so that everyone on the team can try it out and catch as many regressions as possible before releasing it to beta next month.

Member

as-cii commented Mar 16, 2017

Okay, so the failure on AppVeyor seems the same we are observing on master right now, so this pull request shouldn't have affected it.

A huge shoutout goes to @Ben3eeE for testing this out and to @ungb for helping me test packages during the jQuery removal. Also, thank you so much @maxbrunsfeld, @nathansobo and @damieng for helping me out with #13254. ⚡️

I will go ahead and 🚢 this so that everyone on the team can try it out and catch as many regressions as possible before releasing it to beta next month.

@as-cii as-cii merged commit e9bf53f into master Mar 16, 2017

2 of 5 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/branch AppVeyor build failed
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@as-cii as-cii deleted the as-ns-startup-snapshot branch Mar 16, 2017

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Mar 16, 2017

Contributor

👏👏👏 Bravo

Contributor

nathansobo commented Mar 16, 2017

👏👏👏 Bravo

@maxbrunsfeld

This comment has been minimized.

Show comment
Hide comment
@maxbrunsfeld

maxbrunsfeld Mar 16, 2017

Contributor

Amazing!

Contributor

maxbrunsfeld commented Mar 16, 2017

Amazing!

@jedwards1211

This comment has been minimized.

Show comment
Hide comment
@jedwards1211

jedwards1211 Jun 10, 2018

@as-cii why on earth does this cache using the relative path, instead of the absolute path here?
This makes it much more difficult to reload modules in cases where it is necessary (e.g. reloading morpher-transforms.js for atom-morpher and it burned hours of my time trying to debug today. If absolute paths were used as the cache keys it would be much easier to delete from this cache.

jedwards1211 commented on static/index.js in e453b04 Jun 10, 2018

@as-cii why on earth does this cache using the relative path, instead of the absolute path here?
This makes it much more difficult to reload modules in cases where it is necessary (e.g. reloading morpher-transforms.js for atom-morpher and it burned hours of my time trying to debug today. If absolute paths were used as the cache keys it would be much easier to delete from this cache.

This comment has been minimized.

Show comment
Hide comment
@jedwards1211

jedwards1211 Jun 10, 2018

also why do third-party packages wind up calling this monkeypatched require instead of the main one? Shouldn't only core packages that are part of the V8 snapshot need to be redirected to this require implementation, and all third-party code still use the root, non-monkeypatched require implementation?

jedwards1211 replied Jun 10, 2018

also why do third-party packages wind up calling this monkeypatched require instead of the main one? Shouldn't only core packages that are part of the V8 snapshot need to be redirected to this require implementation, and all third-party code still use the root, non-monkeypatched require implementation?

This comment has been minimized.

Show comment
Hide comment
@Arcanemagus

Arcanemagus Jun 10, 2018

Contributor

Please file a new issue to discuss this, comments on a commit aren't the best place to go into this.

Contributor

Arcanemagus replied Jun 10, 2018

Please file a new issue to discuss this, comments on a commit aren't the best place to go into this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment