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

Allow the user to set julia-args for precompile execution #931

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Drvi
Copy link
Contributor

@Drvi Drvi commented Mar 20, 2024

So they can run their potentially expensive precompile execution scripts with -O0.

Not sure if hardcoding -O0 would be preferable... I'm open to suggestions!

@NHDaly
Copy link
Member

NHDaly commented Mar 20, 2024

Not sure if hardcoding -O0 would be preferable... I'm open to suggestions!

It seems reasonable to me that PackageCompiler should just hardcode -O0 during the first pass over the snoop file, independently from letting the user pass custom julia-args. I don't see any reason to run the snoop file with anything other than -O0.

@Drvi
Copy link
Contributor Author

Drvi commented Mar 20, 2024

I guess some precompilation workloads are not particularly light computationally and could take longer without optimizations... But yeah maybe -O0 should be the default 👍

Copy link

codecov bot commented Mar 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.62%. Comparing base (8cd96a1) to head (80c4211).

❗ Current head 80c4211 differs from pull request most recent head feacfe0. Consider uploading reports for the commit feacfe0 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #931   +/-   ##
=======================================
  Coverage   84.62%   84.62%           
=======================================
  Files           3        3           
  Lines         826      826           
=======================================
  Hits          699      699           
  Misses        127      127           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@NHDaly
Copy link
Member

NHDaly commented Mar 20, 2024

I guess some precompilation workloads are not particularly light computationally and could take longer without optimizations... But yeah maybe -O0 should be the default 👍

ah true, good point. 👍

src/PackageCompiler.jl Outdated Show resolved Hide resolved
Copy link
Member

@NHDaly NHDaly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@NHDaly
Copy link
Member

NHDaly commented Mar 25, 2024

Are the tests on Nightly unrelated?

@NHDaly NHDaly requested a review from KristofferC March 25, 2024 01:11
@Drvi
Copy link
Contributor Author

Drvi commented Mar 25, 2024

I checked a couple of nighly failures and there seems to be a bounds error:
BoundsError(a=Array{Base.Partr.taskheap, 1}(dims=(0,), mem=Memory{Base.Partr.taskheap}(0, 0x7f3fe5b3e0b0)[]), i=(0,))

@sjkelly
Copy link
Collaborator

sjkelly commented Mar 27, 2024

This is a good idea. My hope is that propagating and adding all sorts of these configurations will be easier with, #929

Can we split the difference and do -O1?

@@ -834,7 +843,8 @@ function create_app(package_dir::String,
sysimage_build_args::Cmd=``,
include_transitive_dependencies::Bool=true,
include_preferences::Bool=true,
script::Union{Nothing, String}=nothing)
script::Union{Nothing, String}=nothing,
precompile_execution_args::Cmd=`-O0`)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we split the difference and do -O1?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems plausible to me, but what is that based on, @sjkelly? My gut is that -O0 would be better for almost all snoop workloads, since the developer is already pretty strongly incentivized to keep data sizes small, and thus to have compilation-dominated workloads.

What leads you to prefer -O1?

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