Skip to content
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

8129776: The optimized Stream returned from Files.lines should unmap the mapped byte buffer (if created) when closed #2229

Closed
wants to merge 4 commits into from

Conversation

@bplb
Copy link
Member

@bplb bplb commented Jan 25, 2021

Please review this proposed change to unmap the mapped buffer shared between the root and sub-spliterators used in the optimized Stream implementation returned by Files.lines(). A reference counter is added to track the total number of spliterators sharing the buffer. It is set to 1 when the shared buffer is created, and incremented each time a sub-spliterator is created. It is decremented when traversing begins or when the spliterator is closed. If the counter is zero after it is decremented then the shared buffer is unmapped.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8129776: The optimized Stream returned from Files.lines should unmap the mapped byte buffer (if created) when closed

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2229/head:pull/2229
$ git checkout pull/2229

…the mapped byte buffer (if created) when closed
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jan 25, 2021

👋 Welcome back bpb! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr label Jan 25, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jan 25, 2021

@bplb To determine the appropriate audience for reviewing this pull request, one or more labels corresponding to different subsystems will normally be applied automatically. However, no automatic labelling rule matches the changes in this pull request. In order to have an "RFR" email sent to the correct mailing list, you will need to add one or more applicable labels manually using the /label pull request command.

Applicable Labels
  • 2d
  • awt
  • beans
  • build
  • compiler
  • core-libs
  • hotspot
  • hotspot-compiler
  • hotspot-gc
  • hotspot-jfr
  • hotspot-runtime
  • i18n
  • javadoc
  • jdk
  • jmx
  • kulla
  • net
  • nio
  • security
  • serviceability
  • shenandoah
  • sound
  • swing

@bplb
Copy link
Member Author

@bplb bplb commented Jan 26, 2021

/label add nio

@openjdk openjdk bot added the nio label Jan 26, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jan 26, 2021

@bplb
The nio label was successfully added.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Jan 26, 2021

new FileChannelLinesSpliterator(fc, cs, 0, (int) length);
return StreamSupport.stream(fcls, false)
.onClose(Files.asUncheckedRunnable(fc))
.onClose(() -> { fcls.close(); });
Copy link
Contributor

@RogerRiggs RogerRiggs Jan 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typically, the brackets {} are omitted for a single expression.

@openjdk
Copy link

@openjdk openjdk bot commented Jan 26, 2021

@bplb This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8129776: The optimized Stream returned from Files.lines should unmap the mapped byte buffer (if created) when closed

Reviewed-by: rriggs, psandoz, alanb

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 296 new commits pushed to the master branch:

  • ab65d53: 8261261: The version extra fields needs to be overridable in jib-profiles.js
  • 20d7713: 8261334: NMT: tuning statistic shows incorrect hash distribution
  • 92c6e6d: 8261254: Initialize charset mapping data lazily
  • 351d788: 8259074: regex benchmarks and tests
  • d6d5d9b: 8261231: Windows IME was disabled after DnD operation
  • 29a428f: 8261229: MethodData is not correctly initialized with TieredStopAtLevel=3
  • 48c932e: 8231286: HTML font size too large with high-DPI scaling and W3C_LENGTH_UNITS
  • dbc35f6: 8261094: Open javax/swing/text/html/CSS/4765271/bug4765271.java
  • db0ca2b: 8261161: Clean up warnings in hotspot/jtreg/vmTestbase tests
  • 2c28e36: 8237352: Update DatagramSocket to add support for joining multicast groups
  • ... and 286 more: https://git.openjdk.java.net/jdk/compare/5d8861b03d06bf3670c6e052e11a93b7f8894e35...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Jan 26, 2021
@PaulSandoz
Copy link
Member

@PaulSandoz PaulSandoz commented Jan 26, 2021

Looks good. There is some complexity because the Spliterator can be obtained from the Stream returned by Files.lines(). This is not a common case but unfortunately we need to deal with it, otherwise we could just unmap when the Stream is closed. There are edge cases where by splits may occur but they are not traversed, but i don't think we should be concerned about. This is on a good best-effort basis to free up the resource.

Is unmapping via nioAccess.unmapper(b).unmap(); wired up?

In Buffer:

                @Override
                public UnmapperProxy unmapper(ByteBuffer bb) {
                    if (bb instanceof MappedByteBuffer) {
                        return ((MappedByteBuffer)bb).unmapper();
                    } else {
                        return null;
                    }
                }

In MappedByteBuffer:

    UnmapperProxy unmapper() {
        return fd != null ?
                new UnmapperProxy() {
                    @Override
                    public long address() {
                        return address;
                    }

                    @Override
                    public FileDescriptor fileDescriptor() {
                        return fd;
                    }

                    @Override
                    public boolean isSync() {
                        return isSync;
                    }

                    @Override
                    public void unmap() {
                        throw new UnsupportedOperationException();
                    }
                } : null;
    }

I don't see where unmapper is overridden.

@bplb
Copy link
Member Author

@bplb bplb commented Jan 27, 2021

No, unmapping via nioAccess.unmapper(b).unmap() is not wired up. Investigating why.

@mcimadamore
Copy link
Contributor

@mcimadamore mcimadamore commented Jan 27, 2021

No, unmapping via nioAccess.unmapper(b).unmap() is not wired up. Investigating why.

MappedByteBuffer::unmapper is only used (AFAIK) from AbstractMemorySegmentImpl::ofBuffer. When creating a segment view over a buffer, we need to recover mapping info - but close() on the returned segment is a no-op. So we don't need the unmap() method to be wired up - although it doesn't hurt for it to be wired up, if desired.

// non-null to null, either when traversing begins or if the spliterator is
// closed before traversal. If the count is zero after decrementing, then
// the buffer is unmapped.
private AtomicInteger bufRefCount;
Copy link
Contributor

@AlanBateman AlanBateman Feb 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be final but otherwise the changes are okay..

@bplb
Copy link
Member Author

@bplb bplb commented Feb 8, 2021

/integrate

@openjdk openjdk bot closed this Feb 8, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Feb 8, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Feb 8, 2021

@bplb Since your change was applied there have been 299 commits pushed to the master branch:

  • ad525bc: 8261281: Linking jdk.jpackage fails for linux aarch32 builds after 8254702
  • 2fd8ed0: 8240632: Note differences between IEEE 754-2019 math lib special cases and java.lang.Math
  • ace8f94: 8195744: Avoid calling ClassLoader.checkPackageAccess if security manager is not installed
  • ab65d53: 8261261: The version extra fields needs to be overridable in jib-profiles.js
  • 20d7713: 8261334: NMT: tuning statistic shows incorrect hash distribution
  • 92c6e6d: 8261254: Initialize charset mapping data lazily
  • 351d788: 8259074: regex benchmarks and tests
  • d6d5d9b: 8261231: Windows IME was disabled after DnD operation
  • 29a428f: 8261229: MethodData is not correctly initialized with TieredStopAtLevel=3
  • 48c932e: 8231286: HTML font size too large with high-DPI scaling and W3C_LENGTH_UNITS
  • ... and 289 more: https://git.openjdk.java.net/jdk/compare/5d8861b03d06bf3670c6e052e11a93b7f8894e35...master

Your commit was automatically rebased without conflicts.

Pushed as commit 7451962.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@bplb bplb deleted the 8129776-File-lines-Stream branch Feb 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants