You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Bruce Schuchardt <bs...@pivotal.io> on 2016/05/31 22:15:49 UTC
Review Request 48095: GEODE-1468 client/server messaging can create
large objects
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48095/
-----------------------------------------------------------
Review request for geode, Hitesh Khamesra and Udo Kohlmeyer.
Bugs: GEODE-1468
https://issues.apache.org/jira/browse/GEODE-1468
Repository: geode
Description
-------
After a Message has been sent we invoke clear() on each Part contained by the Message. This was nulling out the "part" variable of the Part objects but if one of these "parts" was a HeapDataOutputStream it might hold a list of large buffers. This change set alters Part to close these streams so that their buffers can be cleared.
Diffs
-----
geode-core/src/main/java/com/gemstone/gemfire/internal/HeapDataOutputStream.java 20d01da880f2786a01ee4d4bd64681cd646acd31
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/tier/sockets/Message.java a011875d4ea9aa9a14ac96e568fe6bba464bca89
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/tier/sockets/Part.java bf90fab4999ce96adced9574678605d2bf8a903a
geode-core/src/test/java/com/gemstone/gemfire/internal/cache/tier/sockets/MessageJUnitTest.java 9f05aa7a0ae0d433ff9675a5fbcffc9c98ce8e7b
Diff: https://reviews.apache.org/r/48095/diff/
Testing
-------
New test, precheckin
Thanks,
Bruce Schuchardt
Re: Review Request 48095: GEODE-1468 client/server messaging can
create large objects
Posted by Udo Kohlmeyer <uk...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48095/#review135739
-----------------------------------------------------------
Ship it!
Ship It!
- Udo Kohlmeyer
On May 31, 2016, 10:15 p.m., Bruce Schuchardt wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48095/
> -----------------------------------------------------------
>
> (Updated May 31, 2016, 10:15 p.m.)
>
>
> Review request for geode, Hitesh Khamesra and Udo Kohlmeyer.
>
>
> Bugs: GEODE-1468
> https://issues.apache.org/jira/browse/GEODE-1468
>
>
> Repository: geode
>
>
> Description
> -------
>
> After a Message has been sent we invoke clear() on each Part contained by the Message. This was nulling out the "part" variable of the Part objects but if one of these "parts" was a HeapDataOutputStream it might hold a list of large buffers. This change set alters Part to close these streams so that their buffers can be cleared.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/internal/HeapDataOutputStream.java 20d01da880f2786a01ee4d4bd64681cd646acd31
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/tier/sockets/Message.java a011875d4ea9aa9a14ac96e568fe6bba464bca89
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/tier/sockets/Part.java bf90fab4999ce96adced9574678605d2bf8a903a
> geode-core/src/test/java/com/gemstone/gemfire/internal/cache/tier/sockets/MessageJUnitTest.java 9f05aa7a0ae0d433ff9675a5fbcffc9c98ce8e7b
>
> Diff: https://reviews.apache.org/r/48095/diff/
>
>
> Testing
> -------
>
> New test, precheckin
>
>
> Thanks,
>
> Bruce Schuchardt
>
>
Re: Review Request 48095: GEODE-1468 client/server messaging can
create large objects
Posted by Hitesh Khamesra <hk...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48095/#review136024
-----------------------------------------------------------
Ship it!
Ship It!
- Hitesh Khamesra
On May 31, 2016, 10:15 p.m., Bruce Schuchardt wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48095/
> -----------------------------------------------------------
>
> (Updated May 31, 2016, 10:15 p.m.)
>
>
> Review request for geode, Hitesh Khamesra and Udo Kohlmeyer.
>
>
> Bugs: GEODE-1468
> https://issues.apache.org/jira/browse/GEODE-1468
>
>
> Repository: geode
>
>
> Description
> -------
>
> After a Message has been sent we invoke clear() on each Part contained by the Message. This was nulling out the "part" variable of the Part objects but if one of these "parts" was a HeapDataOutputStream it might hold a list of large buffers. This change set alters Part to close these streams so that their buffers can be cleared.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/internal/HeapDataOutputStream.java 20d01da880f2786a01ee4d4bd64681cd646acd31
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/tier/sockets/Message.java a011875d4ea9aa9a14ac96e568fe6bba464bca89
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/tier/sockets/Part.java bf90fab4999ce96adced9574678605d2bf8a903a
> geode-core/src/test/java/com/gemstone/gemfire/internal/cache/tier/sockets/MessageJUnitTest.java 9f05aa7a0ae0d433ff9675a5fbcffc9c98ce8e7b
>
> Diff: https://reviews.apache.org/r/48095/diff/
>
>
> Testing
> -------
>
> New test, precheckin
>
>
> Thanks,
>
> Bruce Schuchardt
>
>
Re: Review Request 48095: GEODE-1468 client/server messaging can
create large objects
Posted by Bruce Schuchardt <bs...@pivotal.io>.
> On June 1, 2016, 5:40 p.m., Hitesh Khamesra wrote:
> > geode-core/src/main/java/com/gemstone/gemfire/internal/HeapDataOutputStream.java, line 373
> > <https://reviews.apache.org/r/48095/diff/1/?file=1402773#file1402773line373>
> >
> > We were doing this.chunks = null, so jvm should claim chunks no?, I am missing something here..
>
> Bruce Schuchardt wrote:
> A user wants us to clear the chunks list to aid in GC. Their client JVM threw an OOME and had two of these lists consuming most of the heap, though there were no references to them.
>
> Hitesh Khamesra wrote:
> Just browse client code and it seems we are calling clear method from multiple places. May be we should call from "AbstractOp" after reading the whole message.
>
> Bruce Schuchardt wrote:
> After reading the Message it holds Parts that the command classes need to access to deserialize the message components. We can't clear it out until all of the components have been read. Currently server-side clearing is done just before filling the Message with new contents in read().
>
> It might be an improvement to null out the entries in the Part array in Message though.
>
> Hitesh Khamesra wrote:
> Server side, we are calling in ServerConnetcion.DoNormalMsg's finally block, this looks fine. But at client side, I see it get called from multiple places.
client-side clears the parts right after transmitting the message, in Message.sendBytes(), so I think the client side is also fine
- Bruce
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48095/#review135818
-----------------------------------------------------------
On May 31, 2016, 10:15 p.m., Bruce Schuchardt wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48095/
> -----------------------------------------------------------
>
> (Updated May 31, 2016, 10:15 p.m.)
>
>
> Review request for geode, Hitesh Khamesra and Udo Kohlmeyer.
>
>
> Bugs: GEODE-1468
> https://issues.apache.org/jira/browse/GEODE-1468
>
>
> Repository: geode
>
>
> Description
> -------
>
> After a Message has been sent we invoke clear() on each Part contained by the Message. This was nulling out the "part" variable of the Part objects but if one of these "parts" was a HeapDataOutputStream it might hold a list of large buffers. This change set alters Part to close these streams so that their buffers can be cleared.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/internal/HeapDataOutputStream.java 20d01da880f2786a01ee4d4bd64681cd646acd31
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/tier/sockets/Message.java a011875d4ea9aa9a14ac96e568fe6bba464bca89
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/tier/sockets/Part.java bf90fab4999ce96adced9574678605d2bf8a903a
> geode-core/src/test/java/com/gemstone/gemfire/internal/cache/tier/sockets/MessageJUnitTest.java 9f05aa7a0ae0d433ff9675a5fbcffc9c98ce8e7b
>
> Diff: https://reviews.apache.org/r/48095/diff/
>
>
> Testing
> -------
>
> New test, precheckin
>
>
> Thanks,
>
> Bruce Schuchardt
>
>
Re: Review Request 48095: GEODE-1468 client/server messaging can
create large objects
Posted by Bruce Schuchardt <bs...@pivotal.io>.
> On June 1, 2016, 5:40 p.m., Hitesh Khamesra wrote:
> > geode-core/src/main/java/com/gemstone/gemfire/internal/HeapDataOutputStream.java, line 373
> > <https://reviews.apache.org/r/48095/diff/1/?file=1402773#file1402773line373>
> >
> > We were doing this.chunks = null, so jvm should claim chunks no?, I am missing something here..
A user wants us to clear the chunks list to aid in GC. Their client JVM threw an OOME and had two of these lists consuming most of the heap, though there were no references to them.
- Bruce
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48095/#review135818
-----------------------------------------------------------
On May 31, 2016, 10:15 p.m., Bruce Schuchardt wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48095/
> -----------------------------------------------------------
>
> (Updated May 31, 2016, 10:15 p.m.)
>
>
> Review request for geode, Hitesh Khamesra and Udo Kohlmeyer.
>
>
> Bugs: GEODE-1468
> https://issues.apache.org/jira/browse/GEODE-1468
>
>
> Repository: geode
>
>
> Description
> -------
>
> After a Message has been sent we invoke clear() on each Part contained by the Message. This was nulling out the "part" variable of the Part objects but if one of these "parts" was a HeapDataOutputStream it might hold a list of large buffers. This change set alters Part to close these streams so that their buffers can be cleared.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/internal/HeapDataOutputStream.java 20d01da880f2786a01ee4d4bd64681cd646acd31
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/tier/sockets/Message.java a011875d4ea9aa9a14ac96e568fe6bba464bca89
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/tier/sockets/Part.java bf90fab4999ce96adced9574678605d2bf8a903a
> geode-core/src/test/java/com/gemstone/gemfire/internal/cache/tier/sockets/MessageJUnitTest.java 9f05aa7a0ae0d433ff9675a5fbcffc9c98ce8e7b
>
> Diff: https://reviews.apache.org/r/48095/diff/
>
>
> Testing
> -------
>
> New test, precheckin
>
>
> Thanks,
>
> Bruce Schuchardt
>
>
Re: Review Request 48095: GEODE-1468 client/server messaging can
create large objects
Posted by Hitesh Khamesra <hk...@pivotal.io>.
> On June 1, 2016, 5:40 p.m., Hitesh Khamesra wrote:
> > geode-core/src/main/java/com/gemstone/gemfire/internal/HeapDataOutputStream.java, line 373
> > <https://reviews.apache.org/r/48095/diff/1/?file=1402773#file1402773line373>
> >
> > We were doing this.chunks = null, so jvm should claim chunks no?, I am missing something here..
>
> Bruce Schuchardt wrote:
> A user wants us to clear the chunks list to aid in GC. Their client JVM threw an OOME and had two of these lists consuming most of the heap, though there were no references to them.
Just browse client code and it seems we are calling clear method from multiple places. May be we should call from "AbstractOp" after reading the whole message.
- Hitesh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48095/#review135818
-----------------------------------------------------------
On May 31, 2016, 10:15 p.m., Bruce Schuchardt wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48095/
> -----------------------------------------------------------
>
> (Updated May 31, 2016, 10:15 p.m.)
>
>
> Review request for geode, Hitesh Khamesra and Udo Kohlmeyer.
>
>
> Bugs: GEODE-1468
> https://issues.apache.org/jira/browse/GEODE-1468
>
>
> Repository: geode
>
>
> Description
> -------
>
> After a Message has been sent we invoke clear() on each Part contained by the Message. This was nulling out the "part" variable of the Part objects but if one of these "parts" was a HeapDataOutputStream it might hold a list of large buffers. This change set alters Part to close these streams so that their buffers can be cleared.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/internal/HeapDataOutputStream.java 20d01da880f2786a01ee4d4bd64681cd646acd31
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/tier/sockets/Message.java a011875d4ea9aa9a14ac96e568fe6bba464bca89
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/tier/sockets/Part.java bf90fab4999ce96adced9574678605d2bf8a903a
> geode-core/src/test/java/com/gemstone/gemfire/internal/cache/tier/sockets/MessageJUnitTest.java 9f05aa7a0ae0d433ff9675a5fbcffc9c98ce8e7b
>
> Diff: https://reviews.apache.org/r/48095/diff/
>
>
> Testing
> -------
>
> New test, precheckin
>
>
> Thanks,
>
> Bruce Schuchardt
>
>
Re: Review Request 48095: GEODE-1468 client/server messaging can
create large objects
Posted by Hitesh Khamesra <hk...@pivotal.io>.
> On June 1, 2016, 5:40 p.m., Hitesh Khamesra wrote:
> > geode-core/src/main/java/com/gemstone/gemfire/internal/HeapDataOutputStream.java, line 373
> > <https://reviews.apache.org/r/48095/diff/1/?file=1402773#file1402773line373>
> >
> > We were doing this.chunks = null, so jvm should claim chunks no?, I am missing something here..
>
> Bruce Schuchardt wrote:
> A user wants us to clear the chunks list to aid in GC. Their client JVM threw an OOME and had two of these lists consuming most of the heap, though there were no references to them.
>
> Hitesh Khamesra wrote:
> Just browse client code and it seems we are calling clear method from multiple places. May be we should call from "AbstractOp" after reading the whole message.
>
> Bruce Schuchardt wrote:
> After reading the Message it holds Parts that the command classes need to access to deserialize the message components. We can't clear it out until all of the components have been read. Currently server-side clearing is done just before filling the Message with new contents in read().
>
> It might be an improvement to null out the entries in the Part array in Message though.
Server side, we are calling in ServerConnetcion.DoNormalMsg's finally block, this looks fine. But at client side, I see it get called from multiple places.
- Hitesh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48095/#review135818
-----------------------------------------------------------
On May 31, 2016, 10:15 p.m., Bruce Schuchardt wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48095/
> -----------------------------------------------------------
>
> (Updated May 31, 2016, 10:15 p.m.)
>
>
> Review request for geode, Hitesh Khamesra and Udo Kohlmeyer.
>
>
> Bugs: GEODE-1468
> https://issues.apache.org/jira/browse/GEODE-1468
>
>
> Repository: geode
>
>
> Description
> -------
>
> After a Message has been sent we invoke clear() on each Part contained by the Message. This was nulling out the "part" variable of the Part objects but if one of these "parts" was a HeapDataOutputStream it might hold a list of large buffers. This change set alters Part to close these streams so that their buffers can be cleared.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/internal/HeapDataOutputStream.java 20d01da880f2786a01ee4d4bd64681cd646acd31
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/tier/sockets/Message.java a011875d4ea9aa9a14ac96e568fe6bba464bca89
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/tier/sockets/Part.java bf90fab4999ce96adced9574678605d2bf8a903a
> geode-core/src/test/java/com/gemstone/gemfire/internal/cache/tier/sockets/MessageJUnitTest.java 9f05aa7a0ae0d433ff9675a5fbcffc9c98ce8e7b
>
> Diff: https://reviews.apache.org/r/48095/diff/
>
>
> Testing
> -------
>
> New test, precheckin
>
>
> Thanks,
>
> Bruce Schuchardt
>
>
Re: Review Request 48095: GEODE-1468 client/server messaging can
create large objects
Posted by Bruce Schuchardt <bs...@pivotal.io>.
> On June 1, 2016, 5:40 p.m., Hitesh Khamesra wrote:
> > geode-core/src/main/java/com/gemstone/gemfire/internal/HeapDataOutputStream.java, line 373
> > <https://reviews.apache.org/r/48095/diff/1/?file=1402773#file1402773line373>
> >
> > We were doing this.chunks = null, so jvm should claim chunks no?, I am missing something here..
>
> Bruce Schuchardt wrote:
> A user wants us to clear the chunks list to aid in GC. Their client JVM threw an OOME and had two of these lists consuming most of the heap, though there were no references to them.
>
> Hitesh Khamesra wrote:
> Just browse client code and it seems we are calling clear method from multiple places. May be we should call from "AbstractOp" after reading the whole message.
After reading the Message it holds Parts that the command classes need to access to deserialize the message components. We can't clear it out until all of the components have been read. Currently server-side clearing is done just before filling the Message with new contents in read().
It might be an improvement to null out the entries in the Part array in Message though.
- Bruce
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48095/#review135818
-----------------------------------------------------------
On May 31, 2016, 10:15 p.m., Bruce Schuchardt wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48095/
> -----------------------------------------------------------
>
> (Updated May 31, 2016, 10:15 p.m.)
>
>
> Review request for geode, Hitesh Khamesra and Udo Kohlmeyer.
>
>
> Bugs: GEODE-1468
> https://issues.apache.org/jira/browse/GEODE-1468
>
>
> Repository: geode
>
>
> Description
> -------
>
> After a Message has been sent we invoke clear() on each Part contained by the Message. This was nulling out the "part" variable of the Part objects but if one of these "parts" was a HeapDataOutputStream it might hold a list of large buffers. This change set alters Part to close these streams so that their buffers can be cleared.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/internal/HeapDataOutputStream.java 20d01da880f2786a01ee4d4bd64681cd646acd31
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/tier/sockets/Message.java a011875d4ea9aa9a14ac96e568fe6bba464bca89
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/tier/sockets/Part.java bf90fab4999ce96adced9574678605d2bf8a903a
> geode-core/src/test/java/com/gemstone/gemfire/internal/cache/tier/sockets/MessageJUnitTest.java 9f05aa7a0ae0d433ff9675a5fbcffc9c98ce8e7b
>
> Diff: https://reviews.apache.org/r/48095/diff/
>
>
> Testing
> -------
>
> New test, precheckin
>
>
> Thanks,
>
> Bruce Schuchardt
>
>
Re: Review Request 48095: GEODE-1468 client/server messaging can
create large objects
Posted by Hitesh Khamesra <hk...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48095/#review135818
-----------------------------------------------------------
geode-core/src/main/java/com/gemstone/gemfire/internal/HeapDataOutputStream.java (line 373)
<https://reviews.apache.org/r/48095/#comment200860>
We were doing this.chunks = null, so jvm should claim chunks no?, I am missing something here..
- Hitesh Khamesra
On May 31, 2016, 10:15 p.m., Bruce Schuchardt wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48095/
> -----------------------------------------------------------
>
> (Updated May 31, 2016, 10:15 p.m.)
>
>
> Review request for geode, Hitesh Khamesra and Udo Kohlmeyer.
>
>
> Bugs: GEODE-1468
> https://issues.apache.org/jira/browse/GEODE-1468
>
>
> Repository: geode
>
>
> Description
> -------
>
> After a Message has been sent we invoke clear() on each Part contained by the Message. This was nulling out the "part" variable of the Part objects but if one of these "parts" was a HeapDataOutputStream it might hold a list of large buffers. This change set alters Part to close these streams so that their buffers can be cleared.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/internal/HeapDataOutputStream.java 20d01da880f2786a01ee4d4bd64681cd646acd31
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/tier/sockets/Message.java a011875d4ea9aa9a14ac96e568fe6bba464bca89
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/tier/sockets/Part.java bf90fab4999ce96adced9574678605d2bf8a903a
> geode-core/src/test/java/com/gemstone/gemfire/internal/cache/tier/sockets/MessageJUnitTest.java 9f05aa7a0ae0d433ff9675a5fbcffc9c98ce8e7b
>
> Diff: https://reviews.apache.org/r/48095/diff/
>
>
> Testing
> -------
>
> New test, precheckin
>
>
> Thanks,
>
> Bruce Schuchardt
>
>