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.

Sign up for GitHub

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

Jump to bottom

faster invalid object lookup in conservative gc #50599

Merged
merged 2 commits into from
Jul 22, 2023
Merged

faster invalid object lookup in conservative gc #50599

merged 2 commits into from
Jul 22, 2023

Conversation

d-netto
Copy link
Member

@d-netto d-netto commented Jul 19, 2023

We may possibly optimize the free-list lookup by just looking at the page metadata and GC bits.

If we're allocating from a free-list then we have three cases for a valid object:

  1. Object is in a page whose free-list has been exhausted (the meta->nfree == 0 covers this).
  2. Object is in a page whose free-list has not been used since the last GC (the GC bits for the object will be set since it's not freshly allocated, and the cell->bits.gc check should cover this).
  3. Object is in a page whose free-list is currently being used. If it's before the free-list pointer then it's a valid object. If it's after the free-list pointer then it will have its GC bits set and the cell->bits.gc check should cover this.

This PR basically implements case 3 so that we don't have to walk the whole free-list looking for an invalid object.

CC: @fingolfin

@d-netto d-netto requested a review from vtjnash July 19, 2023 17:33
@d-netto d-netto added GC Garbage collector kind:embarrassing-bugfix Whoops! backport 1.10 Change should be backported to the 1.10 release labels Jul 19, 2023
@vtjnash
Copy link
Sponsor Member

vtjnash commented Jul 19, 2023

Since that description is exactly what the comment for this case says this code is supposed to do, it sounds fine to me to remove this again.

@vtjnash
Copy link
Sponsor Member

vtjnash commented Jul 19, 2023

Oh, wait, no, it seems like you are missing logic here

        // We know now that the age bit reflects liveness status during
         // the last sweep and that the cell has not been reused since.
         if (!(meta->ages[obj_id / 8] & (1 << (obj_id % 8)))) {
             return NULL;

@d-netto
Copy link
Member Author

d-netto commented Jul 19, 2023

seems like you are missing logic here

Yeah, with the changes from #49644 we can guarantee that any valid object in the current page we're allocating and past the free-list pointer has the age bit set.

fingolfin
fingolfin approved these changes Jul 20, 2023
View reviewed changes
src/gc.c Outdated Show resolved Hide resolved
@oscardssmith oscardssmith added the performance Must go faster label Jul 20, 2023
@d-netto d-netto force-pushed the dcn/faster-conservative-gc branch from 720b7c6 to 2c31ad2 Compare July 20, 2023 20:16
@d-netto d-netto force-pushed the dcn/faster-conservative-gc branch from 2c31ad2 to b4c9e42 Compare July 21, 2023 22:55
@d-netto d-netto merged commit c82656d into master Jul 22, 2023
5 of 6 checks passed
@d-netto d-netto deleted the dcn/faster-conservative-gc branch July 22, 2023 00:55
KristofferC pushed a commit that referenced this pull request Jul 24, 2023
@d-netto @KristofferC
gc: faster invalid object lookup in conservative GC (#50599)
85301f9 
Optimizes invalid object lookup in conservative GC by just looking at the
page metadata and GC bits.

(cherry picked from commit c82656d)
@KristofferC KristofferC mentioned this pull request Jul 24, 2023
Merged
45 tasks
KristofferC added a commit that referenced this pull request Jul 24, 2023
@KristofferC
release-1.10: Backports for 1.10-alpha2 (#50508)
f9b2614 
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
This was referenced Jul 27, 2023
Merged
Closed
@lgoettgens lgoettgens mentioned this pull request Jul 28, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fingolfin fingolfin fingolfin approved these changes

@vtjnash vtjnash Awaiting requested review from vtjnash

Assignees
No one assigned
Labels
GC Garbage collector kind:embarrassing-bugfix Whoops! performance Must go faster
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@d-netto @vtjnash @fingolfin @KristofferC @oscardssmith