[GitHub] [geode] dschneider-pivotal opened pull request #3200: GEODE-6414: TypeRegistry defineType will now return an existing PdxType

["dschneider-pivotal (GitHub)" <gi...@apache.org>]

Thank you for submitting a contribution to Apache Geode.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?

- [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?

- [ ] Is your initial contribution a single, squashed commit?

- [ ] Does `gradlew build` run cleanly?

- [ ] Have you written or updated unit tests to verify your changes?

- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?

### Note:
Please ensure that once the PR is submitted, you check travis-ci for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.


[ Full content available at: https://github.com/apache/geode/pull/3200 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] dschneider-pivotal commented on pull request #3200: GEODE-6414: PdxInstanceFactory uses too much memory when it creates a PdxInstance

["dschneider-pivotal (GitHub)" <gi...@apache.org>]

I have updated the pull request with a new version of toByteBuffer that takes a startPosition which will get rid of some extra copying.

[ Full content available at: https://github.com/apache/geode/pull/3200 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] jchen21 commented on pull request #3200: GEODE-6414: PdxInstanceFactory uses too much memory when it creates a PdxInstance

["jchen21 (GitHub)" <gi...@apache.org>]

I am not sure about the computational cost here. If the memory saving of `copyRemainingBytes` justifies the computational cost, it is good. And only `PdxInstanceFactory.create()` uses `copyRemainingBytes`. Shall we invoke `copyRemainingBytes` only on certain conditions? e.g. when remaining bytes is less than half of the capacity?

[ Full content available at: https://github.com/apache/geode/pull/3200 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] dschneider-pivotal commented on pull request #3200: GEODE-6414: PdxInstanceFactory uses too much memory when it creates a PdxInstance

["dschneider-pivotal (GitHub)" <gi...@apache.org>]

It would not make the code any faster. So I think it is better to have simple expressions instead of more complex ones.

[ Full content available at: https://github.com/apache/geode/pull/3200 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] dschneider-pivotal commented on pull request #3200: GEODE-6414: PdxInstanceFactory uses too much memory when it creates a PdxInstance

["dschneider-pivotal (GitHub)" <gi...@apache.org>]

When creating a PdxInstance using a PdxInstanceFactory then I think it is okay to say we always want the ByteBuffer to be as small as possible. But for large PdxInstances it is possible that the ByteBuffer has already been allocated to just be big enough for all the bytes. So it is probably worth adding a check for that and not doing a copy in that case.

[ Full content available at: https://github.com/apache/geode/pull/3200 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] jchen21 commented on pull request #3200: GEODE-6414: PdxInstanceFactory uses too much memory when it creates a PdxInstance

["jchen21 (GitHub)" <gi...@apache.org>]

Will it help if we chain the methods like `registry.defineType(type).getTypeId()`? This will save one temp variable.

[ Full content available at: https://github.com/apache/geode/pull/3200 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] dschneider-pivotal commented on issue #3200: GEODE-6414: PdxInstanceFactory uses too much memory when it creates a PdxInstance

["dschneider-pivotal (GitHub)" <gi...@apache.org>]

The one failure was a known issue seen before: https://issues.apache.org/jira/browse/GEODE-2458

[ Full content available at: https://github.com/apache/geode/pull/3200 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] dschneider-pivotal closed pull request #3200: GEODE-6414: PdxInstanceFactory uses too much memory when it creates a PdxInstance

["dschneider-pivotal (GitHub)" <gi...@apache.org>]

[ pull request closed by dschneider-pivotal ]

[ Full content available at: https://github.com/apache/geode/pull/3200 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org