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

SnoopCompile #225

Merged
merged 31 commits into from
Jan 29, 2020
Merged

SnoopCompile #225

merged 31 commits into from
Jan 29, 2020

Conversation

aminya
Copy link
Contributor

@aminya aminya commented Dec 1, 2019

Ready to get merged

Atom infer time is enhanced by 19% 🚀

@pfitzseb
Copy link
Member

pfitzseb commented Dec 1, 2019

Can you check by how much does this improve package load times/time to first eval?

I was under the impression that precompile statements that reach into dependencies don't really work (i.e. aren't cached in ji files), so it's possible that most of the files added here won't make much of a difference.

@aminya
Copy link
Contributor Author

aminya commented Dec 1, 2019

Yes, only "deps/SnoopCompile/precompile/precompile_Atom.jl" is added to precompile now.
I added other files just for the record.

@aminya
Copy link
Contributor Author

aminya commented Dec 1, 2019

Also, ideally this should run by Travis and the jl files inside precompile folder should be updated. I ran this on the latest registered version.

@pfitzseb
Copy link
Member

pfitzseb commented Dec 1, 2019

Oh ok, I didn't scroll down all the way :)

So I guess we'd ideally merge this, open PRs against the bigger dependencies (JSON, Tokenize and CSTParser, probably), and get rid of a bunch of random dependencies (Hiccup, Media, CodeTools, Lazy).

I'll play around with this a bit later today or tomorrow, but thanks for taking the initiative on this! :)

@aviatesk
Copy link
Member

aviatesk commented Dec 1, 2019

just for info: we should care about this kind of regression: bicycle1885/TableReader.jl#58

@aminya
Copy link
Contributor Author

aminya commented Dec 1, 2019

Can you guys run the benchmark from the file that I added? I have a hard time running the benchmark from outside of the Atom. Running it from inside the atom gives the wrong information as it is already loaded.

@aminya
Copy link
Contributor Author

aminya commented Dec 1, 2019

just for info: we should care about this kind of regression: bicycle1885/TableReader.jl#58

I added an issue for creating a Github action to automatically update the precompile files
timholy/SnoopCompile.jl#37

@aviatesk
Copy link
Member

aviatesk commented Dec 1, 2019

Yes, only "deps/SnoopCompile/precompile/precompile_Atom.jl" is added to precompile now.
I added other files just for the record.

could you please ignore/rm those files ? They look pretty messy as is.

@aminya
Copy link
Contributor Author

aminya commented Dec 1, 2019

Yes, only "deps/SnoopCompile/precompile/precompile_Atom.jl" is added to precompile now.
I added other files just for the record.

could you please ignore/rm those files ? They look pretty messy as is.

Cleaned up ♻️

@aminya
Copy link
Contributor Author

aminya commented Dec 2, 2019

I added Github actions for automatically creating the precompile file. Could you guys test it?

@aminya aminya force-pushed the master branch 2 times, most recently from e9d6e05 to d127e73 Compare December 12, 2019 12:36
@aminya
Copy link
Contributor Author

aminya commented Dec 12, 2019

@aminya aminya force-pushed the master branch 2 times, most recently from a95f9e8 to c2bbfcc Compare December 28, 2019 17:16
@codecov
Copy link

codecov bot commented Dec 28, 2019

Codecov Report

Merging #225 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #225   +/-   ##
=======================================
  Coverage   46.07%   46.07%           
=======================================
  Files          40       40           
  Lines        1910     1910           
=======================================
  Hits          880      880           
  Misses       1030     1030
Impacted Files Coverage Δ
src/Atom.jl 100% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6cbb812...06601ab. Read the comment docs.

@aminya

This comment has been minimized.

@aminya
Copy link
Contributor Author

aminya commented Dec 28, 2019

I fixed the issue by test_skiping one of the tests. However, there are still a couple of precompile sentences to filter out

@aminya aminya force-pushed the master branch 6 times, most recently from bad9922 to 6aadc52 Compare December 30, 2019 22:53
@aminya
Copy link
Contributor Author

aminya commented Dec 30, 2019

When I add precompilation sentences generated by Atom some of the tests fail (from modules.jl and goto.jl).

https://github.com/aminya/Atom.jl/commit/6aadc52f978fef0ebefa0c115364e8458482ddce/checks?check_suite_id=379165136

@aminya aminya force-pushed the master branch 4 times, most recently from 5006278 to b87a374 Compare December 31, 2019 01:05
@aminya aminya force-pushed the master branch 12 times, most recently from 6b2ce2c to c9b7f50 Compare January 26, 2020 23:11
@aminya aminya force-pushed the master branch 3 times, most recently from 1af920f to 9858ffd Compare January 26, 2020 23:26
@aminya
Copy link
Contributor Author

aminya commented Jan 26, 2020

Yay! the problem is fixed. Atom infer time is enhanced by 19% 🚀
The PR can be merged now

https://github.com/aminya/Atom.jl/runs/409940740?check_suite_focus=true

I didn't benchmark the actual timing but infer time is reduced by 3.6s which is 19%.

@aminya aminya requested a review from aviatesk January 26, 2020 23:45
@pfitzseb pfitzseb merged commit 066d8a6 into JunoLab:master Jan 29, 2020
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

Successfully merging this pull request may close these issues.

3 participants