Skip to content
This repository was archived by the owner on Oct 12, 2022. It is now read-only.
/ druntime Public archive
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions src/core/memory.d
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,12 @@ private
extern (C) BlkInfo_ gc_query( void* p ) pure nothrow;
extern (C) GC.Stats gc_stats ( ) nothrow @nogc;

// Alias used because DMD thinks the delegate are extern(C) as well otherwise
alias CollectionStartHook = void delegate() nothrow @nogc;
alias CollectionEndHook = void delegate(size_t, size_t) nothrow @nogc;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mihails-strasuns-sociomantic mentioned that this could be two instances of the GC.Stats struct.
However, those are not so cheap to get, since we iterate over the pools (unless I misunderstood something).
Given the use cases for those hooks, I think having a no-cost approach is quite mandatory.

Choose a reason for hiding this comment

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

I think having a no-cost approach is quite mandatory

It still has to hold GC lock which immediately makes it pretty far from no-cost.

Choose a reason for hiding this comment

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

I think acquiring a lock (specially in single threaded environments, where is basically an atomic op usually) is considered no-cost compared to traverse the whole pool set. Also, there seems to be no reason to do it when the user can get the stats anyway with just one extra call, if needed.

Choose a reason for hiding this comment

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

I have two unrelated concerns about it:

  1. It is not cheap in multi-threaded environments and while single-threaded optimization is indeed most suitable to our needs, druntime version should be more uniformly useful.

  2. To avoid calculating exact stats this PR right now exposes freedLargePages and freedSmallPages as monitored values and I would argue those alone have rather limited usefulness.

What would be really cool is to accept different types of callback delegates and provide required stats based on what is desired by callback. Not sure if it can fit into API elegantly though.

extern (C) void gc_monitor ( CollectionStartHook on_start,
CollectionEndHook on_end) nothrow @nogc;

extern (C) void gc_addRoot( in void* p ) nothrow @nogc;
extern (C) void gc_addRange( in void* p, size_t sz, const TypeInfo ti = null ) nothrow @nogc;

Expand Down Expand Up @@ -680,6 +686,23 @@ struct GC
return gc_stats();
}

/**
* Provides a mean to hook the GC on the beggining and end of a collection
*
* If one of the event is of no interest, `null` can be provided.
*
* Params:
* on_start = A delegate to call whenever a collection starts
* on_end = A delegate to call whenever a collection ends.
* The first argument is the number of bytes freed overall,
* the second the number of bytes freed within full pages.
*/
static void monitor(CollectionStartHook on_start, CollectionEndHook on_end)
nothrow @nogc
{
gc_monitor(on_start, on_end);
}

/**
* Adds an internal root pointing to the GC memory block referenced by p.
* As a result, the block referenced by p itself and any blocks accessible
Expand Down
11 changes: 11 additions & 0 deletions src/gc/gcinterface.d
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,10 @@ struct Range

interface GC
{
/// Provided for implementation's convenience, not intended for public usage
protected alias CollectionStartHook = void delegate() nothrow @nogc;
/// Ditto
protected alias CollectionEndHook = void delegate(size_t, size_t) nothrow @nogc;

Choose a reason for hiding this comment

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

@__future? Although I don't think it's working for aliases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think that's necessary for alias actually. If someone defines something with the same name, it will just shadow this.


/*
*
Expand Down Expand Up @@ -148,6 +152,13 @@ interface GC
*/
core.memory.GC.Stats stats() nothrow;

/**
* Track beginning and end of allocation
* Useful for debugging and tuning.
*/
void monitor (CollectionStartHook on_start, CollectionEndHook on_end)
nothrow @nogc;

Choose a reason for hiding this comment

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

@__future?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense.
I would be extremely surprised if someone was implementing this interface, but I suppose that's a good test anyway.

Choose a reason for hiding this comment

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

Yeah, I know it would be extremely unlikely, but this is a good example for what future was created, to avoid any kind of surprises as much as possible, and OTOH we know people is creative and we always get surprised about how assertions like "probably nobody is doing X" prove themselves wrong with time :)

Choose a reason for hiding this comment

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

I don't think @future makes any sense here - gc package is not available to import, it is strictly contained within druntime. So there is no downstream to care about.


/**
* add p to list of roots
*/
Expand Down
19 changes: 18 additions & 1 deletion src/gc/impl/conservative/gc.d
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,10 @@ class ConservativeGC : GC
__gshared size_t line;
__gshared char* file;

/// Hook used for debugging application-side
__gshared CollectionStartHook on_collect_start;
__gshared CollectionEndHook on_collect_end;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

One point raised by @mihails-strasuns-sociomantic is that having delegate + __gshared is dangerous. Since we have the lock it should be okay, but I'm interested to know if there's something addressing this issue anywhere else.

Choose a reason for hiding this comment

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

Well, from pure theoretical PoV it should require shared delegate but I still have no idea if that is even implemented to mean something in D frontend.


Gcx *gcx; // implementation

import core.internal.spinlock;
Expand Down Expand Up @@ -1200,6 +1204,16 @@ class ConservativeGC : GC
return ret;
}

override void monitor (CollectionStartHook on_start, CollectionEndHook on_end)
nothrow @nogc
{
static void store (CollectionStartHook start, CollectionEndHook end)
{
on_collect_start = start;
on_collect_end = end;
}
runLocked!(store)(on_start, on_end);
}

//
//
Expand Down Expand Up @@ -2392,6 +2406,8 @@ struct Gcx
}

debug(COLLECT_PRINTF) printf("Gcx.fullcollect()\n");
if (ConservativeGC.on_collect_start !is null)
ConservativeGC.on_collect_start();
//printf("\tpool address range = %p .. %p\n", minAddr, maxAddr);

{
Expand Down Expand Up @@ -2456,6 +2472,8 @@ struct Gcx

updateCollectThresholds();

if (ConservativeGC.on_collect_end !is null)
ConservativeGC.on_collect_end(freedLargePages, freedSmallPages);
return freedLargePages + freedSmallPages;
}

Expand Down Expand Up @@ -3443,4 +3461,3 @@ unittest
GC.free(z);
GC.minimize(); // release huge pool
}

5 changes: 5 additions & 0 deletions src/gc/impl/manual/gc.d
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,11 @@ class ManualGC : GC
return typeof(return).init;
}

/// This implementation is a no-op as there is no collection to monitor
override void monitor (CollectionStartHook, CollectionEndHook) nothrow @nogc
{
}

void addRoot(void* p) nothrow @nogc
{
roots.insertBack(Root(p));
Expand Down