- User Since
- May 26 2014, 12:49 PM (212 w, 2 d)
Related question: Is the laziness done to save memory, startup time, or
Updated with suggestions from chandler. I'm not sure why all my tests passed even despite having that bug that he noticed. Would be nice if there were a way to write a test for that case, but I'm not sure how. I also made the cleanup suggested to eliminate the extra cost of copying the task. This was a little bit annoying, because I had to go back to using the callable object instead of using the lambda so that I could get move semantics of the callable. But this means I can't easily capture this, so I had to move the callable up to the class level so that I could make it a friend to give it access to TaskQueue's internal state. Anyway, it all works now.
Long term I think one potentially interesting possibility for solving a lot of these threading and lazy evaluation issues is to make a task queue that runs all related operations on a single thread and returns a future you can wait on. This way, the core data structures themselves do not need synchronization because they are only ever accessed from one thread. I actually have a patch in progress to make this kind of infrastructure, which I actually needed for a different reason, but it would work equally well here.
In that case maybe FunctionExtras.h in keeping with the llvm naming
Not to suggest the obvious, but UniqueFunction.h?
Sorry this slipped by me. Lgtm and thanks for the fix!
Tue, Jun 19
This gets the LLVM_ENABLE_THREADS=0 case working. It does so by taking advantage of pointer invalidation properties of std::deque, so I can save off a pointer to the newly inserted element and just run tasks until the pointers match.
Updated based on suggestions from Chandler. I don't have the LLVM_ENABLE_THREADS=0 case working yet, but wanted to make sure this is generally what you had in mind.
Yea I'm not really able to get anything working using the suggested method of the local class. The Task class here doesn't have any logic to wait on the parent task to complete, and furthermore we just return a future from a promise without using async with deferred launch, so waiting on the future doesn't run the function. All of this is fixable, but ultimately you just end up getting pretty close to what I already have I think.
Mon, Jun 18
I can't get your suggestion to work, it's very bizarre. It's claiming that I can't even *move* construct a std::function because Task has a deleted copy constructor.
Sat, Jun 16
Fri, Jun 15
Made the test cases a little stronger. Before I was simply assigning to the local variables in the test case. Now I increment them. This way the test will fail if a task were to ever get executed more than once, for example, whereas before it would pass since it would just be re-assigning the same value.
Added 2 more tests, the first of which exposed a bug in the wait() function. All the wait() function was doing was joining the work thread, but this is incorrect. What it's supposed to do is simply drain the queue. i.e. keep processing work until the queue is empty. It's not supposed to join the thread. Joining the thread doesn't work anyway since we weren't setting the EnableFlag to be false, so this method was deadlocking.
It can assert when we pass the python or clang directory enumeration. We
could also remove the values from the internal enumeration but keep them in
the public enumeration. However, we can’t be forced into providing a stable
internal api just because we must provide a stable public api.
The internal api has no guarantees as to its stability.
Wed, Jun 13
Tue, Jun 12
Looks like good cleanup to me, thanks!
Mon, Jun 11
Alright, if there is no way to get rid of the Duplicates then we can live with them. As you said, the proposed solution is already better than before. I mildly prefer just never setting the delete on close flag in the first place because the case that it address is so rare that it almost doesn't even matter IMO, and I think just reducing the surface area of the Windows' kernels code paths that we touch is a win given how subtle this bug was and how long it took us to figure out. But if you think we should have it then I won't make a big fuss over it :)
We can’t transfer ownership of the handle in that case? With the flag
cleared we shouldn’t have to worry about the pending delete error
Do we have any kind of guarantee that one entity will outlive all other entities that may attempt to share the file?
FWIW, I'm no longer convinced this is a Windows bug based on the reading of the blog post linked in comment 52 of the crbug.
Interestingly, I've been working on LLVM's low level process launching APIs to make it so we can hopefully replace some of this code with that. I don't think it's quite there yet, but hopefully we can converge soon. I'll keep this test case in mind.
Sun, Jun 10
Sat, Jun 9
Fri, Jun 8
Thu, Jun 7
Also, one thing that's a little strange here is that this now marks the one and only Support test that is *not* a unit test. Is that weird? I feel like this is a much better pattern for testing process launching though, so I'd feel pretty bad if I had to re-write all this as unit tests.
FWIW turns out this doesn't work as nicely as I thought it did. So I'm going to bench this one for now. createUniqueFile uses this flag, and there are several places in the codebase that assume they can refer to the path afterwords, which breaks here as this effectively makes an open unliked file on posix platforms. I'll think about the best way to handle this in the future, but it's not high priority right now.
It would be better if you're using a monorepo, that way both parts can just be one single patch.
As a general rule, lld-link is command line compatible with MSVC and
clang-cl is command line compatible with cl. So, the /order option should
work exactly the same with lld-link as it does with link.
Wed, Jun 6
Do you just need a pdb, or does it really need to be a vs pdb? lld can
generate high quality pdbs now. So it might be possible to use lld to link
and produce a pdb when you run the test.
The problem is that Phabricator doesn't preserve binary in diffs
Tue, Jun 5
Mon, Jun 4
Changes from first revision:
Sun, Jun 3
rsplit(char) already exists. That said, another way to reuse code would be
to have rsplit(char) call the StringRef version
Fri, Jun 1
I trust that this doesn't regress the test suite on Windows. You guys are more active in the Windows stuff for the time being, so if this works for you and doesn't break anything, I'm good with it.
Thu, May 31
Wed, May 30
+1 I’d like to get rid of all EnumerateXXX with callback functions and
replace with iterator based equivalents. Given that in this case the
iterator version already exists, I definitely think we should try to use it