Skip to content

Conversation

@jpsamaroo
Copy link
Contributor

Adapts to changes in JuliaLang/julia#41742, and requires JuliaLang/julia#42162

@jpsamaroo
Copy link
Contributor Author

Aside: This supports indirect usage of FlameGraphs by Dagger through ProfileSVG, which doesn't appear to need any changes to account for the MT profiling changes.

@codecov
Copy link

codecov bot commented Sep 8, 2021

Codecov Report

Merging #42 (96da799) into master (92bb8d7) will decrease coverage by 2.57%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #42      +/-   ##
==========================================
- Coverage   93.65%   91.07%   -2.58%     
==========================================
  Files           5        5              
  Lines         268      269       +1     
==========================================
- Hits          251      245       -6     
- Misses         17       24       +7     
Impacted Files Coverage Δ
src/graph.jl 93.61% <66.66%> (-0.95%) ⬇️
src/FlameGraphs.jl 0.00% <0.00%> (-46.67%) ⬇️

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 92bb8d7...96da799. Read the comment docs.

@timholy
Copy link
Owner

timholy commented Sep 8, 2021

I guess this will fail nightly until the 2nd PR is merged. If I don't notice, feel free to ping me, @jpsamaroo. Thanks for doing this!

@jpsamaroo jpsamaroo marked this pull request as draft September 8, 2021 18:31
@jpsamaroo
Copy link
Contributor Author

I think we can wait for the Julia PR to be merged first so we don't mess up CI. It gives me a bit more time to test this with Dagger and make sure I didn't miss anything.

@timholy timholy closed this Sep 17, 2021
@timholy timholy reopened this Sep 17, 2021
@timholy timholy marked this pull request as ready for review October 3, 2021 07:27
@timholy timholy merged commit af6e33c into timholy:master Oct 3, 2021
@timholy
Copy link
Owner

timholy commented Oct 3, 2021

This doesn't pass but it's a step in the right direction. Let's merge it and move to the next issue.

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.

2 participants