-
Notifications
You must be signed in to change notification settings - Fork 104
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.
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
test for concurrent text drawing #126
Conversation
So it illustrate how to fix data race. We have to integrate your code in the default font cache. |
For library users, the two globals to think about are FontCache and GlyphCache. FontCache already offers library users the option of supplying an interface implementation that synchronizes map access. That's what I did in the example. In the PR What isn't in the library is an equivalent mechanism for GlyphCache - so the test case is intended as an illustration for the GlyphCache problem. I'm not sure that's the end of it, though: as an experiment I disabled the glyph cache altogether and still ran into race conditions. I should add that it's entirely possible my TestFontCache as given in the PR is faulty. I'd be tempted to make both caches properties of the context struct, but that's too far from the current approach. I believe @redstarcoder is looking into a mechanism that allows the user to protect GlyphCache in the way FontCache can already be protected. I've started to try something similar, but I'm sure @redstarcoder's version will be much better as I'm new to the codebase. |
thanks @gerald1248, |
@llgcode, @redstarcoder: I've simplified the test case so it just calls the library and doesn't attempt any trickery with either the font cache or the glyph cache. Having spent some time mulling this over I really think that a potential race condition when calling a 2D drawing library from a server handler is a bug. On balance I think the default needs to be threadsafe. Setters like SetFontCache and SetGlyphCache are great, but they should be the tools to squeeze out the last bit of performance when concurrency isn't a requirement. In other words: the library really needs to pass |
Currently the library isn't thread-safe by default and hasn't ever been, this makes me think that most users expect this behaviour at this point. I also don't see it as a huge issue as I always check if my libraries are thread-safe or not by default anyways when I'm dealing with multi-threaded applications (and I think others usually do the same...). This isn't to say that things can't change, as we do want to refactor areas of code for a future release, I just don't think this is an easy one to justify. If we did make it thread-safe by default we'd have to have new caches generated with each If we have new caches generated with each Mutex locks are another option, but they're hackish to me as a default here. Not only will they reduce performance in single-threaded applications, we don't have any data by how much, or how much of an increase in speed a mutex locked multi-threaded application observes. Ideally for 4 threads you'd have 4 caches, problem solved (but I don't see any obvious way the library could do that for you...). Using channels is hopefully something we never do, but it's there as an option. With the current model, it'd be slower than mutex locks (they work like fancy mutex locks). I disagree that the library should be thread-safe by default. Unless you can propose a solution that doesn't result in a loss of performance in single-threaded applications (what seems to be the most common use), or at least one that results in a model we can all be happy with. As it stands new caches with each GC hurts both, mutex locks hurt single-threaded (and aren't ideal for multi-threaded), and channels are pretty much out of the scope completely. I think we should make it clear in the header of the documentation that draw2d isn't thread-safe by default. That way people at least won't expect it to be thread-safe. Maybe we could even throw in an example of a multi-threaded application too so newer programmers won't have to come up with their own code. |
hi @redstarcoder, thanks for taking the time to reply in detail. I can see where you're coming from. I'm not suggesting not having a glyph cache or a font cache; they're both great optimizations. For my use case fussing over thread safety is overkill - I only have one context, use it and then return the base64 encoded string of the image in the request response. (Not loading fonts from disk is really important to me and something the font cache enables for me.) The PR wasn't working as it was, so I made it a simple test that only passes if default thread safety is a goal. If it isn't, I do understand and that's fine too. |
Hi @redstarcoder @gerald1248 , |
Happy New Year! @llgcode alright well I'm even more glad I wrote out a breakdown of options then! In PR #128 I can implement a local cache and abolish the global default. Then I think we just have to do another PR for localising @gerald1248 looks like you're getting exactly what you wanted anyways :). This is now clearly in the scope of the project. Have you noticed any other globals we need to worry about? |
@redstarcoder I haven't but once the font and glyph caches are local to the context, |
Yes we can create another PR to remove global font cache to understand the impact and communicate on this if needed. I don't see other globals. |
Run with -race switch:
$ go test -race sync_test.go
See #121 and #122