You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@activemq.apache.org by franz1981 <gi...@git.apache.org> on 2018/08/15 16:07:17 UTC
[GitHub] activemq-artemis pull request #2250: ARTEMIS-1996 MappedSequentialFileFactor...
GitHub user franz1981 opened a pull request:
https://github.com/apache/activemq-artemis/pull/2250
ARTEMIS-1996 MappedSequentialFileFactory may cause DirectByteBuffer off-heap memory leaks
Compaction is now reusing direct ByteBuffers on both
reading and writing with explicit and deterministic
release to avoid high peak of native memory utilisation
after compaction.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/franz1981/activemq-artemis mmap_ro_2_read_journal
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/activemq-artemis/pull/2250.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #2250
----
commit efab802be791f4142c3f05543dd7da6b8469e82f
Author: Francesco Nigro <ni...@...>
Date: 2018-08-15T12:39:41Z
ARTEMIS-1996 MappedSequentialFileFactory may cause DirectByteBuffer off-heap memory leaks
Compaction is now reusing direct ByteBuffers on both
reading and writing with explicit and deterministic
release to avoid high peak of native memory utilisation
after compaction.
----
---
[GitHub] activemq-artemis issue #2250: ARTEMIS-1996 MappedSequentialFileFactory may c...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on the issue:
https://github.com/apache/activemq-artemis/pull/2250
I have included the same optimizations while loading the journal, not only on compaction
---
[GitHub] activemq-artemis pull request #2250: ARTEMIS-1996 MappedSequentialFileFactor...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2250#discussion_r220699668
--- Diff: artemis-journal/src/main/java/org/apache/activemq/artemis/core/journal/impl/JournalImpl.java ---
@@ -1796,15 +1826,15 @@ private synchronized JournalLoadInformation load(final LoaderCallback loadManage
private void checkID(final long id) {
if (id > maxID.longValue()) {
- maxID.set(id);
+ maxID.lazySet(id);
--- End diff --
After just dealing with a bunch of concurrency issues (bit worried) how sure is it we def have only single thread at this point with the callbacks, e.g. how safe is it to switch these to lazySet from set?
---
[GitHub] activemq-artemis issue #2250: ARTEMIS-1996 MappedSequentialFileFactory may c...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/2250
Can you rebase this? lots of commits since you sent this. I will merge it along with the other PR on the journal.
---
[GitHub] activemq-artemis issue #2250: ARTEMIS-1996 MappedSequentialFileFactory may c...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on the issue:
https://github.com/apache/activemq-artemis/pull/2250
@michaelandrepearce @clebertsuconic I'm on vacation ATM, hence I need help to test it on CI and obviously review/test it guys :+1:
---
[GitHub] activemq-artemis pull request #2250: ARTEMIS-1996 MappedSequentialFileFactor...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/activemq-artemis/pull/2250
---
[GitHub] activemq-artemis pull request #2250: ARTEMIS-1996 MappedSequentialFileFactor...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2250#discussion_r220720824
--- Diff: artemis-journal/src/main/java/org/apache/activemq/artemis/core/journal/impl/JournalImpl.java ---
@@ -1796,15 +1826,15 @@ private synchronized JournalLoadInformation load(final LoaderCallback loadManage
private void checkID(final long id) {
if (id > maxID.longValue()) {
- maxID.set(id);
+ maxID.lazySet(id);
--- End diff --
It's a fair concern, but If we are not single threaded here I'm quite sure that both set/lazySet are wrong, given that none of them is addressing the chance that others will do the same concurrently :)
But looking at the code maxID is being created and used with single threaded semantic here, quoting the comments on the code:
`// AtomicLong is used only as a reference, not as an Atomic value
final AtomicLong maxID = new AtomicLong(-1); final AtomicLong maxID = new AtomicLong(-1);`
---
[GitHub] activemq-artemis pull request #2250: ARTEMIS-1996 MappedSequentialFileFactor...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2250#discussion_r227024184
--- Diff: tests/unit-tests/src/test/java/org/apache/activemq/artemis/tests/unit/core/journal/impl/fakes/FakeSequentialFileFactory.java ---
@@ -417,6 +418,18 @@ public synchronized void writeDirect(final ByteBuffer bytes, final boolean sync,
}
+ @Override
+ public synchronized void blockingWriteDirect(ByteBuffer bytes,
--- End diff --
do you really need synchronized here? this is synchronizing the factory.
---
[GitHub] activemq-artemis issue #2250: ARTEMIS-1996 MappedSequentialFileFactory may c...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/2250
@franz1981 could you rebase? Hopefully also doing this will trigger a new build, as last build failed.
---