[GitHub] activemq-artemis pull request #2015: ARTEMIS-1807 File-based Large Message e...

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
20 messages Options
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2015: ARTEMIS-1807 File-based Large Message e...

clebertsuconic-3
GitHub user franz1981 opened a pull request:

    https://github.com/apache/activemq-artemis/pull/2015

    ARTEMIS-1807 File-based Large Message encoding should use read-only mmap

    It provides a LargeBodyEncoder using a read only mapped view of the large
    message file to encode the data to be sent. It saves unecessary copies and
    context switches while consuming the encoded data.

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/franz1981/activemq-artemis ARTEMIS-1807

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/activemq-artemis/pull/2015.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 #2015
   
----
commit d3a221f99e30a7ba39898f0623fdd9a65c42d134
Author: Francesco Nigro <nigro.fra@...>
Date:   2018-04-15T15:17:19Z

    ARTEMIS-1807 File-based Large Message encoding should use read-only mmap
   
    It provides a LargeBodyEncoder using a read only mapped view of the large
    message file to encode the data to be sent. It saves unecessary copies and
    context switches while consuming the encoded data.

----


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #2015: ARTEMIS-1807 File-based Large Message encoding...

clebertsuconic-3
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2015
 
    @clebertsuconic @gaohoward AFAIK you've worked recently on Large messages: please share your thoughts and if you see other points where this improvement should be used.
   
    The optimal solution would be by using [DefaultFileRegion](https://netty.io/4.1/api/io/netty/channel/DefaultFileRegion.html) to zero-copy data through the wire, but it involves to change `Packet` in order to use a `FileRegion` body instead of `byte[]`: this solution I've proposed seems less invasive, while providing very good perf improvements.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #2015: ARTEMIS-1807 File-based Large Message encoding...

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2015
 
    @clebertsuconic Do not merge it yet, I have an exception on the test `BridgeTest.testBridgeWithVeryLargeMessage`


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #2015: ARTEMIS-1807 File-based Large Message encoding...

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2015
 
    @clebertsuconic I'm thinking to re-implement it by dividing the mmap in several mapped chunks instead of just 1 big one to help the OS to be able to map it when it is > Integer.MAX_VALUE


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #2015: ARTEMIS-1807 File-based Large Message encoding...

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2015
 
    `JournalStorageManager::addBytesToLargeMessage` is another point that need to be improved to avoid OOM and better performances too


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #2015: ARTEMIS-1807 File-based Large Message encoding...

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user gaohoward commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2015
 
    I think using mapped file will get better performance over the NIO channel read.
    It would be better to have a unit test with it. @clebertsuconic wdyt?


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #2015: ARTEMIS-1807 File-based Large Message encoding...

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user clebertsuconic commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2015
 
    @franz1981 I think so too.. but I would do it in the context of a refactoring in AMQP also. we need to properly implement large messages in AMQP.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #2015: ARTEMIS-1807 File-based Large Message encoding...

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2015
 
    @gaohoward
   
    > I think using mapped file will get better performance over the NIO channel read.
   
    @orpiske have some nice idea to test it for sure :P
   
    > It would be better to have a unit test with it. @clebertsuconic wdyt?
   
    @clebertsuconic @gaohoward
    If you 2 will vote for the chunked version I will squash the 2 commits and provide a unit test to cover this new implementation: ATM it is just using the tests related to large messages.
   
    @clebertsuconic
    > I think so too.. but I would do it in the context of a refactoring in AMQP also
   
    That means that the same large body encoder should be reused for it as well?
   



---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #2015: ARTEMIS-1807 File-based Large Message encoding...

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2015
 
    @franz1981 can you provide a perf test before and after for normal usage of the mapped journal e.g. as the main journal.
   
    I ask as we have ended up using it for a specific use case internally, where we are using it for latency reasons, so i want to be sure theres no degrade for the current usage.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #2015: ARTEMIS-1807 File-based Large Message encoding...

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2015
 
    @michaelandrepearce Good points: paging (the consuming side) , this PR and a mapped journal should contend the OS page cache , so it depends on the dirty/dirty background ratio and total system memory.
    Probably should makes sense to allow this feature to be configurable (and paging as well) in order to avoid contention between them or just tune the OS according (tricky IMO). Wdyt?



---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #2015: ARTEMIS-1807 File-based Large Message encoding...

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2015
 
    @michaelandrepearce
    > we are using it for latency reasons
    Nice!! Happy to hear that :):)  no datasync, no party! :)


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #2015: ARTEMIS-1807 File-based Large Message encoding...

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2015
 
    @michaelandrepearce I'm thinking with @orpiske which kind of test should make sense in order to emulate the correct use case to be stressed...
    Please, can you share your throughts on that?
    I'm concerned about how much is OS configuration sensitive running those tests as well


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #2015: ARTEMIS-1807 File-based Large Message encoding...

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user mtaylor commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2015
 
    @michaelandrepearce If you are using non large messages, and without paging, this should have 0 impact.  @franz1981 can you confirm.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #2015: ARTEMIS-1807 File-based Large Message encoding...

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2015
 
    @mtaylor @michaelandrepearce  Yep and it isn't changing the way the mapped journal works: it uses a different code path from the journal, without changing the MappedSequentialFile in any of its parts.
   
    I agree that I could provide relative measurements to show which impact it has vs the original version: with @orpiske probably we could start providing some relative numbers before merging anything. @orpiske wdyt?


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #2015: ARTEMIS-1807 File-based Large Message encoding...

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2015
 
    @mtaylor , ive had an offline discussion with @franz1981 he kindly went through in detail this change with me, know i better understand +1 from me.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #2015: ARTEMIS-1807 File-based Large Message encoding...

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user orpiske commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2015
 
    @franz1981 we can definitely work out some numbers for this!


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #2015: ARTEMIS-1807 File-based Large Message encoding...

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2015
 
    @michaelandrepearce @mtaylor @clebertsuconic @gaohoward Guys I was looking to the improvements made by this PR and....I found that there is a log of Journal activity with it! I mean....ASYNCIO ones...it makes sense?
    That activity seems the bottleneck on consumer side....


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #2015: ARTEMIS-1807 File-based Large Message encoding...

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2015
 
    That an histogram comparing 2.5 and this PR with 1 MB messages, default consumer window and a target throughput of 50 msg/sec:
    ![1m_50](https://user-images.githubusercontent.com/13125299/39083092-ea2ef82e-455e-11e8-95b4-d088e119596d.png)



---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #2015: ARTEMIS-1807 File-based Large Message encoding...

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2015
 
    I?m clising this in the meantime proper tests will be performed :)


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2015: ARTEMIS-1807 File-based Large Message e...

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user franz1981 closed the pull request at:

    https://github.com/apache/activemq-artemis/pull/2015


---