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

Store heapsnapshot files in tempdir() instead of current directory #50026

Merged

Conversation

adnan-alhomssi
Copy link
Contributor

@adnan-alhomssi adnan-alhomssi commented Jun 1, 2023

Currently, Julia stores the heapsnapshot in the current directory which is sometimes not writeable by the process and also not a good choice.
This PR stores the file in the temporary directory that tempdir() returns.

@adnan-alhomssi adnan-alhomssi changed the title Use tempdir() to store heap snapshot files instead of abspatch ~= roo… Store heapsnapshot files in tempdir() instead of root directory Jun 1, 2023
Copy link
Contributor

@raminammour raminammour left a comment

Choose a reason for hiding this comment

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

Maybe add
@info "Writing snapshot to $f"
before returning, so the user can find the file to upload to Chrome? :)
I leave it to others to review whether the PR is needed/appropriate, but I find these randomly generated file paths impossible to find otherwise.

@NHDaly
Copy link
Member

NHDaly commented Jun 1, 2023

@raminammour's suggestion makes sense to me. I vaguely remembered that it printed the filepath already? Maybe i'm misremembering though.

Also CC: @IanButterworth

@adnan-alhomssi adnan-alhomssi changed the title Store heapsnapshot files in tempdir() instead of root directory Store heapsnapshot files in tempdir() instead of current directory Jun 1, 2023
@KristofferC
Copy link
Sponsor Member

The file path is returned so no need to print it inside the function.

@KristofferC KristofferC merged commit 877b368 into JuliaLang:master Jul 5, 2023
@KristofferC KristofferC added the backport 1.10 Change should be backported to the 1.10 release label Jul 5, 2023
@vchuravy
Copy link
Member

vchuravy commented Jul 5, 2023

This PR should have updated the docstring since it explicitly mentions the storage location.

I would have preferred the option to pass in the storage location, instead of using tmp. These files need to be found by an external program manually and I often want to compare overtime.

@IanButterworth
Copy link
Sponsor Member

IanButterworth commented Jul 5, 2023

Yeah and maybe a better default is a subdir of the primary depot?

@NHDaly
Copy link
Member

NHDaly commented Jul 6, 2023

Valentin: there is already a method that takes the output path. This is just changing the default if you don't provide it.

+1 that we need to update the docstring though

@vchuravy
Copy link
Member

vchuravy commented Jul 6, 2023

This is just changing the default if you don't provide it.

That provides the absolute path though. tempdir is not a useful storage location after the program terminated.

take_heap_snapshot(dir=tempdir()) would have been an API that provides an easy way to customize the storage location.

@vtjnash
Copy link
Sponsor Member

vtjnash commented Jul 6, 2023

tempdir is not a useful storage location after the program terminated

WDYM? tempdir is deleted on reboot on posix (and never cleaned up on Windows due to lack of tmpfs)

@vchuravy
Copy link
Member

vchuravy commented Jul 6, 2023

I lose the correlation of what I am working on and what the snapshot belongs to. On restart I lose the file.

@IanButterworth
Copy link
Sponsor Member

What about default to cd and if it's unwritable warn and write to tempdir ?

@KristofferC
Copy link
Sponsor Member

KristofferC commented Jul 6, 2023

So use the API then where you give the path then? I feel like I'm missing something...

Alternatively, remove this API completely.

@vtjnash
Copy link
Sponsor Member

vtjnash commented Jul 6, 2023

We defaulted to pwd before this PR. That is what was changed here.

@NHDaly
Copy link
Member

NHDaly commented Jul 6, 2023

take_heap_snapshot(dir=tempdir()) would have been an API that provides an easy way to customize the storage location.

The trouble is that there's already an API that specifies the full path. I guess we could also add a kwarg for dir= (and name=?) and make the keyword option(s) mutually exclusive with the full path?

Would that be nicer in your opinion?

Is that requested change independent from the change in the default location?

@vchuravy
Copy link
Member

vchuravy commented Jul 6, 2023

Yes, using kwargs to separate dir from filename would be great.

I do think we should revert the default change.

@NHDaly
Copy link
Member

NHDaly commented Jul 6, 2023

I think the concern with the current default of writing to the pwd is that it seems optimized for an interactive project, but is not a good setting for a deployed project, like a cloud-deployment, or a docker image deployment or whatever.

I think you're saying: that's the right choice for us to make, and we should just ask the production deployment scenarios to set a different output directory? I think I am okay with that. I just want to acknowledge the spirit of the change in this PR.

IanButterworth added a commit to IanButterworth/julia that referenced this pull request Jul 8, 2023
KristofferC pushed a commit that referenced this pull request Jul 11, 2023
KristofferC pushed a commit that referenced this pull request Jul 11, 2023
KristofferC added a commit that referenced this pull request Jul 24, 2023
Backported PRs:
- [x] #50411 <!-- Fix weird dispatch of * with zero arguments -->
- [x] #50202 <!-- Remove dynamic dispatch from _wait/wait2 -->
- [x] #50064 <!-- Fix numbered prompt with input only with comment -->
- [x] #50026 <!-- Store heapsnapshot files in tempdir() instead of
current directory -->
- [x] #50402 <!-- Add CPU feature helper function -->
- [x] #50387 <!-- update newpages pointer after actually sweeping pages
-->
- [x] #50424 <!-- avoid potential type-instability in _replace_(str,
...) -->
- [x] #50444 <!-- Optimize getfield lowering to avoid boxing in some
cases -->
- [x] #50474 <!-- docs: Fix a `!!! note` which was miscapitalized -->
- [x] #50466 <!-- relax assertion involving pg->nold to reflect that it
may be a bit in… -->
- [x] #50490 <!-- Fix compat annotation for italic printstyled -->
- [x] #50488 <!-- fix typo in `Base.isassigned` with `Tridiagonal` -->
- [x] #50476 <!-- Profile: Add specifying dir for `take_heap_snapshot`
and handling if current dir is unwritable -->
- [x] #50461 <!-- fix typo in the --gcthreads argument description -->
- [x] #50528 <!-- ssair: Correctly handle stmt insertion at end of basic
block -->
- [x] #50533 <!-- ensure internal_obj_base_ptr checks whether objects
past freelist pointer are in freelist -->
- [x] #49322 <!-- improve cat design / performance -->
- [x] #50540 <!-- gc: remove over-eager assertion -->
- [x] #50542 <!-- gf: remove unnecessary assert cycle==depth -->
- [x] #50559 <!-- Expand kwcall lowering positional default check to
vararg -->
- [x] #50058 <!-- Add unwrapping mechanism for triangular mul and solves
-->
- [x] #50551 <!-- typeintersect: also record chained `innervars` -->
- [x] #50552 <!-- read(io, Char): fix read with too many leading ones
-->
- [x] #50541 <!-- precompile: ensure globals are not accidentally
created where disallowed -->
- [x] #50576 <!-- use atomic compare exchange when setting the GC
mark-bit -->
- [x] #50578 <!-- gf: make method overwrite/delete an error during
precompile -->
- [x] #50516 <!-- Fix visibility of assert on GCC12/13 -->
- [x] #50597 <!-- Fix memory corruption if task is launched inside
finalizer -->
- [x] #50591 <!-- build: fix various makefile bugs -->
- [x] #50599 <!-- faster invalid object lookup in conservative gc -->
- [x] #50634 <!-- 🤖 [master] Bump the SparseArrays stdlib from b4b0e72
to 99c99b4 -->
- [x] #50639 <!-- Backport LLVM patches to fix various issues. -->
- [x] #50546 <!-- Revert storage of method instance in LineInfoNode -->
- [x] #50631 <!-- Shift DCE pass to optimize imaging mode code better
-->
- [x] #50525 <!-- only check that values are finite in `generic_lufact`
when `check=true` -->
- [x] #50587 <!-- isassigned for ranges with BigInt indices -->
- [x] #50144 <!-- Page based heap size heuristics -->


Need manual backport:
- [ ] #50595 <!-- Rename ENV variable `JULIA_USE_NEW_PARSER` ->
`JULIA_USE_FLISP_PARSER` -->



Non-merged PRs with backport label:
- [ ] #50637 <!-- Remove SparseArrays legacy code -->
- [ ] #50618 <!-- inference: continue const-prop' when concrete-eval
returns non-inlineable -->
- [ ] #50598 <!-- only limit types in stack traces in the REPL -->
- [ ] #50594 <!-- Disallow non-index Integer types in isassigned -->
- [ ] #50568 <!-- `Array(::AbstractRange)` should return an `Array` -->
- [ ] #50523 <!-- Avoid generic call in most cases for getproperty -->
- [ ] #50172 <!-- print feature flags used for matching pkgimage -->
@KristofferC KristofferC removed the backport 1.10 Change should be backported to the 1.10 release label Jul 24, 2023
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.

7 participants