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

Add a blacklist for precompile statements, and allow users to override the blacklist #333

Closed
wants to merge 1 commit into from

Conversation

DilumAluthge
Copy link
Member

@DilumAluthge DilumAluthge commented Feb 14, 2020

Closes #329
Related to #295
Related to JuliaLang/julia#31156


This is a replacement for #329. This PR adds a blacklist for precompile statements that are known to crash Julia when you try to compile them into a sysimage. The differences between this PR and #329 are:

  1. In add a blacklist for functions that are known to crash Julia when compiled into sysimage #329, the user cannot customize the blacklist; it is hardcoded. In this PR, there is a default blacklist, but users are able to customize the blacklist if they choose.
  2. In add a blacklist for functions that are known to crash Julia when compiled into sysimage #329, members of the blacklist must be AbstractStrings. In this PR, members of the blacklist may be either AbstractStrings or Regexes. This allows a user to block any precompile statements that match any pattern in a set of given patterns.

cc: @KristofferC

@ianshmean may be interested in this, especially the Regex part (see his comment here: #295 (comment))


Just a quick word about the motivation for this PR. I don't think it makes sense for the default blacklist to, for example, blacklist all permutedims methods. That is way too aggressive for most users. So we want to have a relatively conservative blacklist. But we want users to be able to provide a custom blacklist as well. And because a user may need to, for example, block methods with Array{Bool, N} for all values of N, we need to allow them to pass Regexes for ultimate customization ability. But users that are not experienced with regexes may instead just want to pass a vector of strings. That is why this PR supports both Regexes and AbstractStrings.

@DilumAluthge
Copy link
Member Author

DilumAluthge commented Feb 14, 2020

@ianshmean could you try out this PR and let me know if it solves your issues?

I would recommend using a custom blacklist that looks something like this:

precompile_blacklist = [r"permutedims"i]

This will cause anything with permutedims in it to be blacklisted.

@codecov-io
Copy link

codecov-io commented Feb 14, 2020

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #333   +/-   ##
=======================================
  Coverage   81.18%   81.18%           
=======================================
  Files           2        2           
  Lines         303      303           
=======================================
  Hits          246      246           
  Misses         57       57
Impacted Files Coverage Δ
src/PackageCompiler.jl 80.36% <ø> (ø) ⬆️

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 eadc2ce...d763957. Read the comment docs.

@IanButterworth
Copy link
Member

Seems to be working. It helped me get past permutedims to see that copyto! was causing the same issue, thanks to the LLVM function name report that's on julia master.
Working my way through. I'll share the list once successful

@IanButterworth
Copy link
Member

I haven't managed to skip the copyto! error. It seems to be being invoked by an unknown function.
[r"permutedims"i, r"copyto!"i] doesn't filter it out, nether does r"copy"i

@KristofferC
Copy link
Member

Yeah, I am not sure this is a feasible solution since other functions can probably end up calling it. And copyto! is pretty common...

@DilumAluthge
Copy link
Member Author

@ianshmean What happens if your blacklist is[r"Array{Bool,"i]?

@DilumAluthge
Copy link
Member Author

I wonder if copyto! is being used in the broadcasting machinery.

@IanButterworth
Copy link
Member

I can try that.

I’ve actually found that my package doesn’t fail on an amd64 Linux machine while it does on the aarch64 system I’ve been testing. I’ve outputted all the precompile statements from both and I’m going to do a symdiff on them to figure out what’s different

@IanButterworth
Copy link
Member

IanButterworth commented Feb 16, 2020

Not so simple..

[ Info: Num statements on aarch64: 6406
[ Info: Num statements on amd64: 6096
[ Info: Statements generated on aarch64 but not on amd64: 1537
[ Info: Statements generated on amd64 but not on aarch64: 1277

@aminya
Copy link
Contributor

aminya commented Feb 16, 2020

@DilumAluthge
Copy link
Member Author

DilumAluthge commented Feb 19, 2020

@ianshmean I think you’re going to have to do a bisect on all of the precompile statements on the platform that is failing. Probably run it overnight.

@KristofferC
Copy link
Member

We already kinda did that but AFAIU even if you remove the one that fails, another one can just take its place because running one precompile() line compiles a lot of dependent stuff behind the scenes.

@DilumAluthge
Copy link
Member Author

@KristofferC What are your thoughts on this PR? It seems like it hasn’t actually helped Ian fix his problem. Should we close this PR and hope for a fix from upstream (Julia and/or LLVM)?

@KristofferC
Copy link
Member

I think so yes.

@DilumAluthge DilumAluthge deleted the dpa/blacklist branch February 21, 2020 09:26
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.

5 participants