I can't find this comment in the PR so replying via email ...
On 11/03/2022 9:24 am, Ioi Lam wrote:
On Wed, 9 Mar 2022 07:47:19 GMT, Thomas Stuefe <stu...@openjdk.org> wrote:
Ioi Lam has updated the pull request incrementally with one additional commit
since the last revision:
Fixed zero build
src/hotspot/share/utilities/hashtable.hpp line 42:
40:
41: LP64_ONLY(unsigned int _gap;)
42:
For 64-bit, you now lose packing potential in the theoretical case the
following payload does not have to be aligned to 64 bit. E.g. for T=char, where
the whole entry would fit into 8 bytes. Probably does not matter as long as
entries are allocated individually from C-heap which is a lot more wasteful
anyway.
For 32-bit, I think you may have the same problem if the payload starts with a
uint64_t. Would that not be aligned to a 64-bit boundary too? Whether or not
you build on 64-bit?
I think setting the memory, or at least the first 8..16 bytes, of the entry to zero
in BasicHashtable<F>::new_entry could be more robust:
(16 bytes in case the payload starts with a long double but that may be
overthinking it :)
template <MEMFLAGS F> BasicHashtableEntry<F>*
BasicHashtable<F>::new_entry(unsigned int hashValue) {
char* p = :new (NEW_C_HEAP_ARRAY(char, this->entry_size(), F);
::memset(p, 0, MIN2(this->entry_size(), 16)); // needs reproducable
BasicHashtableEntry<F>* entry = ::new (p) BasicHashtableEntry<F>(hashValue);
return entry;
}
If you are worried about performance, this may also be controlled by a template
parameter, and then you do it just for the system dictionary.
Thanks for pointing this out. I ran more tests and found that on certain
platforms, there are other structures that have problems with uninitialized
gaps. I ended up changing `os::malloc()` to zero the buffer when running with
-Xshare:dump. Hopefully one extra check of `if (DumpSharedSpaces)` doesn't
matter too much for regular VM executions because `os::malloc()` already has a
high overhead.
This is raising red flags for me sorry. Every user of the JDK is now
paying a penalty because of something only needed when dumping the
shared archive. It might not be much but it is the old "death by a
thousand cuts". Is there any way to tell the OS to pre-zero all memory
provided to the current process, such that we could set that when
dumping and not have to check on each allocation?
And I have to wonder how easy it would be to re-introduce
non-deterministic values in these data structures that are being dumped.
Does malloc itself even guarantee to return the same set of addresses
for the same sequence of requests in different executions of a program?
Cheers,
David
-----
-------------
PR: https://git.openjdk.java.net/jdk/pull/7748
