Revival of DCache, now with SHA1 as hash function + dependencies #7843
Conversation
Getting closer to building Phobos with DCache
DmitryOlshansky
requested review from
ibuclaw,
dnadlinger,
MartinNowak,
mathias-lang-sociomantic,
rainers and
WalterBright
as code owners
|
Thanks for your pull request, @DmitryOlshansky! 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. |
| import core.stdc.string; | ||
| import core.atomic; | ||
|
|
||
| version(Posix) |
There was a problem hiding this comment.
Windows has memory-mapped files, too.
| /// Load the cache identified by 'keyspace' | ||
| void initialize(const(char)[] keyspace) | ||
| { | ||
| char[512] buf; |
| void initialize(const(char)[] keyspace) | ||
| { | ||
| char[512] buf; | ||
| const(char)* home = getenv("HOME"); |
There was a problem hiding this comment.
I have no idea what all the following string manipulation and file calls are doing without very carefully reading the code. Please add a block comment.
| struct DCache | ||
| { | ||
| /// Load the cache identified by 'keyspace' | ||
| void initialize(const(char)[] keyspace) |
There was a problem hiding this comment.
Global comment: Please add Ddoc headers for all functions.
| } | ||
| } | ||
|
|
||
| /// Clean the cache |
There was a problem hiding this comment.
What does "cleaning" the cache mean? Deleting it? Filling it with 0's?
| } | ||
| static assert(TocEntry.sizeof == 8); | ||
|
|
||
| static struct Data |
There was a problem hiding this comment.
Data is very generic. What sort of data is it?
| } | ||
| } | ||
|
|
||
| bool put(const(ubyte)[] key, const(ubyte)[] value) |
There was a problem hiding this comment.
Ddoc comment needed. What the function does, what the parameters are, what the return value means.
| { | ||
| uint h = first32bits(key); | ||
| size_t offset = h & (toc.length-1); | ||
| uint itemSize = cast(uint)(Data.sizeof + key.length + value.length); |
There was a problem hiding this comment.
Any possibility of integer overflow or truncation?
| bool put(const(ubyte)[] key, const(ubyte)[] value) | ||
| { | ||
| uint h = first32bits(key); | ||
| size_t offset = h & (toc.length-1); |
There was a problem hiding this comment.
I assume this means toc.length must be a power of 2. I don't see where this is documented.
| lock(); | ||
| scope(exit) unlock(); | ||
| for (;;) | ||
| { |
There was a problem hiding this comment.
There are lots of hashing algorithms. It'd be nice if there was an explanation of which one this is, maybe with a URL to an explanation if it's a standard one.
| shared(pid_t)* owner; // owner of a lock, points to shared memory | ||
| uint* nextAlloc; // last allocated block to avoid linear scan | ||
| pid_t ourPid; // pid of this process | ||
| TocEntry[] toc; // table of contents, fixed-size open-addressing hash map |
There was a problem hiding this comment.
Should document that the length must be a power of 2.
| *owner = 0; | ||
| } | ||
|
|
||
| Data* dataEnd(){ return cast(Data*)(cast(void*)data + DATA_SIZE); } |
There was a problem hiding this comment.
Investigate using attributes. This one can be pure, for example.
| import dmd.dclass; | ||
| import dmd.declaration; | ||
| import dmd.dmodule; | ||
| import dmd.doc; | ||
| import dmd.dimport; |
There was a problem hiding this comment.
why is this being imported?
| if (ti.aliasdecl) | ||
| buf.writestring(ti.aliasdecl.toChars()); | ||
| else | ||
| buf.writestring(ti.name.toChars()); |
There was a problem hiding this comment.
This looks like part of another PR?
| @@ -1543,6 +1546,10 @@ private bool parseCommandLine(const ref Strings arguments, const size_t argc, re | |||
| else if (p[4]) | |||
| goto Lerror; | |||
| } | |||
| else if (strcmp(p + 1, cast(char*)"cache=mmap") == 0) | |||
There was a problem hiding this comment.
please don't use strcmp in mars.d anymore.
| { | ||
| foreach (id; *global.versionids) | ||
| { | ||
| const(char)* s = id.toChars(); |
There was a problem hiding this comment.
use id.toString() and don't use strlen
| bool cacheable = (istate == null || istate.caller == null) | ||
| && !thisarg // if have this then can mutate the object as "side-effect" | ||
| && !fd.isNogc() // compiler bug - should not consider GC in CTFE closures | ||
| && strncmp(fd.ident.toChars(), "__", 2) != 0; |
There was a problem hiding this comment.
strncmp, strlen, strcmp, etc., must die.
| @@ -836,6 +1012,53 @@ private Expression interpret(FuncDeclaration fd, InterState* istate, Expressions | |||
| } | |||
| eargs[i] = earg; | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
The following large chunks of code should be factored out into separate functions.
| @@ -836,6 +1012,53 @@ private Expression interpret(FuncDeclaration fd, InterState* istate, Expressions | |||
| } | |||
| eargs[i] = earg; | |||
| } | |||
|
|
|||
| // Skip generated symbols and only permit caching of top-level CTFE calls | |||
| bool cacheable = (istate == null || istate.caller == null) | |||
There was a problem hiding this comment.
How about having cacheable be off if global.params.cache is off? I.e. why test both repeatedly in subsequent code?
|
As a small security detail, shouldn't you be using SHA2? |
Indeed would make sense, given that such a cache could be used on sites like run.dlang.io. |
|
Just making a list so we can collaborate a bit on this.
|
Getting closer to building Phobos with DCache enabled
Reboot of #7239 moved here because the original discussion got super long.