Re: URGENT: Request for review: 7122222: GC log is limited to 2G for 32-bit

[Mikael Gerdin]

On 2013-06-05 02:21, Daniel D. Daugherty wrote:
OK, based on the largefiles.pdf write-up, your use of _FILE_OFFSET_BITS=64
is going to cause ostream.o to bind to various 64-bit versions of some
functions. Based on my very hazy memory of my days in file system code,
this binding is going to affect ostream.o only unless ostream.o is
writing to some file with the foo64() function and some other code is
also writing to the same file at the same time with foo(). However, if we
have two different functions writing to the same open file at the same
time, then we have bigger issues. :-)

I'm good with these changes now. I agree that solving the problem of
setting _FILE_OFFSET_BITS=64 for the entire VM build doesn't have to
solved right now.

I think this change is really scary, setting _FILE_OFFSET_BITS=64 when 
compiling ostream.cpp will effectively cause the headers to swap out the 
implementations of the f[open,tell,seek] to 64 bit versions for all 
headers that are included and inlined in ostream.cpp.
Other parts of the code using the same headers will see different 
versions of the functions with different parameters due to off_t 
changing sizes.

I think that what we should do is to use the "Transitional compilation 
environment" detailed in §2.6.2 in largefiles.pdf and change the calls 
in ostream.cpp to use the appropriate f[open,tell,seek]64 functions 
directly. I feel this is especially important at this late stage in the 
release to make an explicit change instead of setting a #define which 
has propagating side-effects.

As Tao mentioned this will require us to handle the fact that there is 
no fopen64() call on Windows, that the CRT fopen() already seems to 
handle large files and that ftell64() and fseek64() have slightly 
different names on Windows. I don't think this is a large hurdle and I 
think we know how to solve this problem.

/Mikael


Dan


On 6/4/13 6:06 PM, Tao Mao wrote:
Thank you for review, Dan.

I'll try to answer as much as I can. Please see inline.

Thanks.
Tao

On 6/4/13 4:35 PM, Daniel D. Daugherty wrote:
> http://cr.openjdk.java.net/~tamao/7122222/webrev.00/

Tao,

I think the lack of response to this review request is the absolutely
strange nature of these changes. And I thought I put out some weird
code reviews... :-)

make/linux/makefiles/vm.make
    Build ostream.o with _FILE_OFFSET_BITS==64 on Linux. Nothing
    obvious in this webrev about what this will mean so I took a
    look at src/share/vm/utilities/ostream.{c,h}pp and I see no
    use of _FILE_OFFSET_BITS in either of those source files.

    Must be in the source files somewhere, but I can't find any
    use of _FILE_OFFSET_BITS in the entire hotspot source base.

make/solaris/makefiles/vm.make
Build ostream.o with _FILE_OFFSET_BITS==64 on Solaris.

    OK, I looked for _FILE_OFFSET_BITS in /usr/include on my
    Solaris box. Lots of references, but nothing that helps me
    understand what you're doing here.
src/os/solaris/vm/os_solaris.inline.hpp
    The addition of _FILE_OFFSET_BITS==64 means that the
    os::readdir() function will use the safer, multi-threaded
    version of readdir_r(). Seems fine to me.

Here's what I need to know:

- what effect does _FILE_OFFSET_BITS have on building ostream.{c,h}pp?
_FILE_OFFSET_BITS is set to be picked by c++ compiler.

For why we need to set _FILE_OFFSET_BITS==64 in this case, please
refer to the following document

This Sun White Paper
(http://unix.business.utah.edu/doc/os/solaris/misc/largefiles.pdf)
summarizes the usage of the flags on solaris (page "5-26"). And, it
should apply to Linux the same way as was agreed across platforms
(http://linuxmafia.com/faq/VALinux-kb/2gb-filesize-limit.html).

- if ostream.o has one idea about the value of _FILE_OFFSET_BITS
  what happens if another part of the VM has a different idea about
  the value of _FILE_OFFSET_BITS?
_FILE_OFFSET_BITS is not set for other particular effects, but for
extending the ability to deal with large files in ostream.{c,h}pp. So,
if other files have a different idea about _FILE_OFFSET_BITS, they
can't deal with large files. No more no less.


I saw this in the post to the Runtime alias:

> Included runtime dev to see whether they have some idea to handle
> the compilation choices.

And I still have no idea what you're asking here? What compilation
choices? Are you asking about your Makefile changes? Are asking
about defining _FILE_OFFSET_BITS for the entire build instead of
just one object (ostream.o)? Are you worried that this VM is going
to have mis-matched pieces and be unstable?
"Are asking about defining _FILE_OFFSET_BITS for the entire build
instead of just one object (ostream.o)?" is my main question I
originally tried to ask.


So I reviewed it, but I definitely can't approve it without more
info. I realize that you're up against the RDP2 limit, but this
change has too many open questions (for now)...

BTW, it is not at all clear whether Win32 will be able to write a 2GB+
GC log or not. The conversation below didn't help me at all.
I used a jdk7 (just any) to successfully generate a log file larger
than 4GB. So, it shouldn't be a problem for Win32.

Dan



On 6/4/13 5:03 PM, Tao Mao wrote:
Since the changeset touched makefiles, I've included
build-dev@openjdk.java.net .

I need to push the hsx24 bug asap. Please review it.

Thanks.
Tao

On 6/4/13 2:37 PM, Tao Mao wrote:
Hi all,

Need reviews to catch RDP2.

The current webrev is a working solution to all platforms, Linux,
Windows, and Solaris.
http://cr.openjdk.java.net/~tamao/7122222/webrev.00/

Thanks.
Tao

On 5/30/13 10:21 AM, Tao Mao wrote:
Included runtime dev to see whether they have some idea to handle
the compilation choices.

For now, it's been verified that the fix is functionally sufficient.

Thanks.
Tao

On 5/29/13 5:27 PM, Tao Mao wrote:
Thank you, Mikael.

Please see inline.

Reviewers, please review it based on the following new observation.

Tao

On 5/27/13 2:05 AM, Mikael Gerdin wrote:
Tao,

On 2013-05-25 02:19, Tao Mao wrote:
7ux bug

webrev:
http://cr.openjdk.java.net/~tamao/7122222/webrev.00/

changeset:
(1) make -D_FILE_OFFSET_BITS=64 only available to generating
ostream.o

Why conservative rather than making -D_FILE_OFFSET_BITS=64
globally
applicable?

Global setting of -D_FILE_OFFSET_BITS=64 on linux works fine;
however,
there are at least five code conflicts if introducing the flag
globally
to Solaris.

One was resolved as in os_solaris.inline.hpp, but the rest four
files
had conflicts deep in c library. Even if they are excluded from
setting
-D_FILE_OFFSET_BITS=64, the compiled VM is corrupted.

(2) For now, no Windows solution.
I haven't found any clean solution for solving this problem on
Windows.

This seems like an insufficient fix if you can't make it work on
all platforms. I tried building with "-D_LARGEFILE64_SOURCE
-D_FILE_OFFSET_BITS=64" ons Solaris and hit an #error in
libelf.h saying it wasn't supported so I understand your problem
there.
Yes, that's my grief :( you touched them, a bunch of them. That's
why I chose to apply the flag only to the files (ostream.cpp and
ostream.hpp) I want the effect.

Instead I suggest that you use the compatibility API described
in lf64(5) on Solaris. This API consists of fopen64, ftell64 and
friends and is exposed when "-D_LARGEFILE64_SOURCE" is set.

The same "-D_LARGEFILE64_SOURCE" is available on Linux and has
the added advantage of not changing any existing symbols and
therefore we can set the define for all files instead of just
ostream

This approach has the added advantage that it more closely
resembles the changes which will be needed for Windows anyway.
Those changes would consist of changing calls to ftell/fseek to
64-bit versions and changing fopen to fopen64 on Solaris/Linux.
Both ways have pros and cons. The current implementation excludes
the usage of fopen64, providing portability (since there's no
fopen64 for Windows). Meanwhile, I understand your suggestion
provides other benefits.

This Sun White Paper
(http://unix.business.utah.edu/doc/os/solaris/misc/largefiles.pdf) summarizes
the usage of the flags on solaris (Page 5-26). And, it should
apply to Linux the same way as was agreed across platforms.


Since there is no fopen64 on Windows it seems that the default
fopen already supports large files.
I tested, and you are correct that the 32-bit VM on Windows can
write beyond 2GB (and beyond 4GB). Thank you, it's solved "half
of my problem" :)

/Mikael


test:
(1) Ability to write over 2g file for 32-bit builds were
verified on the
following configurations.

Linux * i586
Solaris * i586
Solaris * sparc

(2) Need a JPRT test for sanity check.