[11/9/15, 7:55:49 AM] Kim Barrett: Chris - see https://bugs.openjdk.java.net/browse/JDK-8131330 and https://bugs.openjdk.java.net/browse/JDK-8039042
[11/9/15, 8:02:16 AM] Chris Thalinger: Makes sense but why are these new files not following any naming conventions?  And wouldn't it make more sense to have it in hotspot/src/share/vm/utilities/?
[11/9/15, 8:06:29 AM] Kim Barrett: Concurrent GCs are doing this somewhat skanky thing - reading and writing blocks of memory concurrently.  That isn't a widespread need, and shouldn't be, so it is in GC-specific area.  If someone came up with a need for it outside GC, it could be moved.
[11/9/15, 8:07:56 AM] Kim Barrett: Regarding file name, what would you suggest?  There's exactly one function, whose name follows the usual naming conventions.  Any other name seemed like it would just make it harder to find.
[11/9/15, 8:07:58 AM] Chris Thalinger: But the problem is that no one outside of GC would know it exists.
[11/9/15, 8:08:20 AM] Chris Thalinger: What if I told you we have the exact same functionality in opto/?
[11/9/15, 8:09:05 AM] Kim Barrett: Well call me surprised...
[11/9/15, 8:09:10 AM] Chris Thalinger: :D
[11/9/15, 8:09:19 AM] Chris Thalinger: Well, we don���t but that���s the point.
[11/9/15, 8:09:34 AM] Chris Thalinger: You wouldn���t look there.
[11/9/15, 8:11:11 AM] John Rose: Sounds like it belongs in src/share/vm/utilities/copy.hpp
[11/9/15, 8:13:59 AM] Kim Barrett: We talked about putting it in copy, but (1) the use case is pretty unusual, (2) the architecture for platform dependency in copy seemed quite unhelpful for the needs of this facility.
[11/9/15, 8:17:21 AM] Chris Thalinger: That���s why we have these: hotspot/src/cpu/<arch>/vm/copy_<arch>.hpp
[11/9/15, 8:18:46 AM] John Rose: Doesn't matter how unusual the use case is; if it's a copy (or fill), and especially if it's got platform-specific logic, it is easier to manage in copy.hpp.
[11/9/15, 8:27:53 AM] John Rose: I'm a little surprised fill_to_memory_atomic wasn't a right choice.
[11/9/15, 8:34:21 AM] Kim Barrett: fill_to_memory_atomic has different semantics.  And while I haven't looked at the generated code, there are a bunch of loops in its implementation that look like they might be subject to compiler transformation into memset, leading to use of block initializing stores (the thing the GC code needs to avoid), which would resurrect the problem the new function is addressing.
[11/9/15, 8:36:36 AM] John Rose: If fill_to_memory_atomic uses BIS the same bug exists there also.  This is another indicator that centralizing the copy functions makes for best management.
[11/9/15, 8:37:49 AM] John Rose: Please file a bug to consolidate those functions into one file.  If there are really special reasons to keep them separate, we can document them on the bug, and therefore have at least a small linkage between the two files.  But I really think the right answer here is consolidation.
[11/9/15, 8:42:27 AM] Kim Barrett: Why is fill_to_memory_atomic using BIS problematic?  It is used in one place, Unsafe_SetMemory.  I haven't yet found the documentation for that, which might provide a hint.
[11/9/15, 8:48:03 AM] Kim Barrett: OK, found that documentation.  No clues there; behavior is described, but no motivation.
[11/9/15, 8:48:31 AM] Kim Barrett: In particular, nothing about concurrent reads and writes.  In fact, nothing that I wouldn't expect a good implementation of memset to already do.
[11/9/15, 8:54:00 AM] Kim Barrett: The platform dependency architecture used by copy has led to large amounts of code duplication.  For the specific function at hand, we'd have a specialized for sparc version, and a generic version that is copied across all other platforms.  That copying is what I was referring to when I said the architecture was not helpful.  I think that's a problem.
[11/9/15, 8:57:41 AM] Chris Thalinger: That���s a general problem with architecture abstractions.
[11/9/15, 8:59:04 AM] Chris Thalinger: We support 4 architectures.  Having:
[11/9/15, 8:59:06 AM] Chris Thalinger: inline void memset_with_concurrent_readers(void* to, int value, size_t size) {
  ::memset(to, value, size);
}
[11/9/15, 8:59:19 AM] Chris Thalinger: in three other files shouldn���t be a big problem.
[11/9/15, 9:00:10 AM] Chris Thalinger: You could also have virtual methods and override them :)
[11/9/15, 9:05:02 AM] Kim Barrett: There are other ways to deal with architecture abstractions than copy-paste reuse, and that don't involve runtime polymorphism.  There are complexities to all of the options, but I really dislike seeing the same code in most of four (actually six, including zero and closed, or maybe more if there are some os variants too) files.  And even worse is when the code is not quite the same, and one is left to wonder whether the differences are significant.
[11/9/15, 9:08:16 AM] Chris Thalinger: Pick one of the ���other ways���.  Anything is better than the current state.
[11/9/15, 9:09:20 AM] John Rose: And yet, that is the way we do business today.  I'd be happy to see it improved.  But for now let's not fragment our copy logic.
[11/9/15, 9:29:54 AM] Kim Barrett: I'm still interested in why fill_to_memory_atomic using BIS is a problem.  The documentation for Unsafe.SetMemory is certainly suggestive of something interesting being intended.  I just don't see a useful inference to draw from that.  And of course, if it is indeed a problem, someone ought to give it a careful look, because I suspect that it is doing so with recent compilers.
[11/9/15, 9:39:31 AM] Mikael Vidstedt: IIRC BIS doesn't guarantee memory ordering, or something like that?
[11/9/15, 9:40:56 AM] Kim Barrett: The problem with BIS is that it first zeros and then sets to the new value.  This breaks some GC uses where a reader expects to see either the old value or the new, but not these "out of thin air" zeros.
[11/9/15, 9:41:59 AM] Kim Barrett: Nothing about fill_to_memory_atomic or memset_with_concurrent_readers involves memory ordering directly.
[11/9/15, 10:08:47 AM] John Rose: Any atomic operation is forbidden to introduce intermediate memory states between the start and final states.  Therefore, fill_to_memory_atomic may not use BIS, and it would be a bug if it did.
[11/9/15, 10:10:00 AM] John Rose: (More subtly, an atomic operation is not allowed to perform multiple writes of the same value.  Thus, even if you are doing zero-fill to contended memory, BIS is wrong.  A racing write might be writing non-zero, and could read the first zero stored by a BIS-y sequence, store a non-zero, and later read the second zero stored by the BIS-y writer.  This only happens rarely, when the BIS-using thread is suspended after a BIS and before a zero-write which "confirms" a BIS-created zero-write.  Not all BIS uses will be this clumsy, but some are.)