-
-
Notifications
You must be signed in to change notification settings - Fork 421
Avoid GC allocation when building the search path #3391
base: master
Are you sure you want to change the base?
Conversation
Thanks for your pull request and interest in making D better, @ryuukk! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please see CONTRIBUTING.md for more information. If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment. Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "master + druntime#3391" |
src/core/sys/windows/stacktrace.d
Outdated
@@ -345,26 +345,28 @@ extern(Windows) BOOL FixupDebugHeader(HANDLE hProcess, ULONG ActionCode, | |||
return FALSE; | |||
} | |||
|
|||
private string generateSearchPath() | |||
private size_t generateSearchPath(char[] path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private size_t generateSearchPath(char[] path) | |
private size_t generateSearchPath(ref char[MAX_PATH] path) |
Otherwise one could pass null
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This size is wrong: the "search path" being generated is a concatenation of semicolon-separated paths each of which individually has (EDIT: presumably) that path limit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then we can't really avoid GC allocations, can we ? We could however use malloc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made some changes after reading your messages, it is now done in 2 pass, 1st to get the size needed, then i create the array with malloc, i don't know if that is how it should be done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess a reasonable stack size would be char[MAX_PATH * defaultPathList.length] temp
. Doing it in a single pass and only resorting to a heap allocation + 2nd pass in case that size doesn't suffice (very unlikely) would be ideal.
@@ -345,26 +345,38 @@ extern(Windows) BOOL FixupDebugHeader(HANDLE hProcess, ULONG ActionCode, | |||
return FALSE; | |||
} | |||
|
|||
private string generateSearchPath() | |||
private char[] generateSearchPathAlloc() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if the point is to avoid GC allocation, please mark is @nogc
so it is obvious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where was the GC allocation happening before?
string stuff I bit of context, i'm trying to enforce a nogc globally using: hack extern (C) __gshared string[] rt_options = ["gcopt=initReserve:0"];
extern (C) void* gc_malloc(size_t sz, uint ba = 0, const TypeInfo = null)
{
import core.stdc.stdio: printf;
import core.stdc.stdlib: abort;
printf("no gc_malloc\n");
abort();
return null;
}
extern (C) void* gc_calloc(size_t sz, uint ba = 0, const TypeInfo = null)
{
import core.stdc.stdio: printf;
import core.stdc.stdlib: abort;
printf("no gc_calloc\n");
abort();
return null;
}
extern (C) auto gc_qalloc(size_t sz, uint ba = 0, const TypeInfo = null)
{
import core.stdc.stdio: printf;
import core.stdc.stdlib: abort;
printf("no gc_qalloc\n");
abort();
return 0;
} Works fine when i don't init the runtime I now want to init the runtime, so i'm trying to remove all the GC allocations, so i can keep use this hack This PR is small because it's my trying to learn the workflow to contribute |
@ryuukk Currently, this is failing the autotester. |
} | ||
path ~= "\0"; | ||
return path; | ||
return []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider return null
ping @ryuukk |
{ | ||
if ( (len = GetEnvironmentVariableA( e.ptr, temp.ptr, temp.length )) > 0 ) | ||
auto buffer = cast(char[]) malloc(total + 1)[0 .. total + 1]; | ||
foreach (e; defaultPathList) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is wasteful to iterate defaultPathList twice. You can integrate this loop in the one above.
No description provided.