Skip to content

implemented LWJGL3 BufferAllocator#588

Merged
empirephoenix merged 5 commits into
jMonkeyEngine:masterfrom
JavaSaBr:added_lwjgl3_allocator
Jan 11, 2017
Merged

implemented LWJGL3 BufferAllocator#588
empirephoenix merged 5 commits into
jMonkeyEngine:masterfrom
JavaSaBr:added_lwjgl3_allocator

Conversation

@JavaSaBr
Copy link
Copy Markdown
Contributor

Implemented LWJGLBufferAllocator.

@empirephoenix
Copy link
Copy Markdown
Contributor

What is the reason for all those changes? Please explain why you think it is necessary to change more than just adding the new class for lwjgl3.

@JavaSaBr
Copy link
Copy Markdown
Contributor Author

JavaSaBr commented Jan 1, 2017

  1. I added the logic which decides what implementation does need to use.
  2. I added a factory because the field "ALLOCATOR" must be final.
  3. I just renamed the logger to LOGGER in the LwjglContext :)

@empirephoenix
Copy link
Copy Markdown
Contributor

Lets start this way:
I'm totally fine with the lwjgl allocator,
I have no issue with the logger renaming, (I would prefer a global code unifiyng at some point, but this will not happen I fear, as the usual discussions about formating already once happend, so I'm ok with this)
I have a slight issue with the new logic, because it prevents multiple applications running at the same time with different allocators. I see no real benefit in having that field static, instead the same logic could be done in the constructor of the bufferutils. (In general I'm not a big fan of static initializers, and static fields)

Please note that this is my personal opinion, I'm not entirely sure how the rest of the jme community thinks about using statics here.

@pspeed42
Copy link
Copy Markdown
Contributor

pspeed42 commented Jan 1, 2017

Yeah, logger to LOGGER is a no-no. In general, don't change unnecessary things if you want your patch accepted.

@JavaSaBr
Copy link
Copy Markdown
Contributor Author

JavaSaBr commented Jan 1, 2017

so, what do I need to do else? Do I need to revert the name of the logger field?

@JavaSaBr
Copy link
Copy Markdown
Contributor Author

JavaSaBr commented Jan 1, 2017

@empirephoenix but at this moment, the field is static and we can't use different allocators at the same time.

@empirephoenix
Copy link
Copy Markdown
Contributor

Oh you are right, my bad.

I would suggest:

  1. rename the logger back (i wouldn't care but other obviously do)
  2. Couldn't the logic to determine the correct allocator being done in the BufferUtils, instead of using a different class for this? After all it is less than a few dozen lines, and could replace the current logics way.
  3. You do change behavior , many users would currently use the Reflection allocator, however if that one could not be loaded, for example due to problems with JavaSecurity stuff, jdk9 beta version ect. it would switch to the primitive one. This means that for most users (running jdk8 in a normal environment) the allocator does change. Just doing your if property not set the old way (try the reflection one before) would solve this.

@JavaSaBr
Copy link
Copy Markdown
Contributor Author

JavaSaBr commented Jan 1, 2017

  1. This ensures that we will not have cases, when we have allocated buffers from different allocators because it can make some issues for developers when they will try to destroy these.
  2. My logic offers that you will use LWJGL Allocator only in the one case when you use LWJGL3, and I think it is the best option because it is a native allocator of the LWJGL3. If you use another backend, you will use Reflection/Primitive Allocator. You can determine a default allocator for each backend and override it using system property.

@JavaSaBr
Copy link
Copy Markdown
Contributor Author

JavaSaBr commented Jan 1, 2017

I have updated the PR.

@empirephoenix
Copy link
Copy Markdown
Contributor

Great, thanks one last question:

You set the property in the lwjgl context, what happens if it is already set by something else before? Eg a custom made allocator for a special case, would this overwrite the users property? Maybee a check should be added, to only set it, if none is set.

@JavaSaBr
Copy link
Copy Markdown
Contributor Author

JavaSaBr commented Jan 1, 2017

I agree with you and I have updated the PR :)

@empirephoenix
Copy link
Copy Markdown
Contributor

OK, looks fine for me, I will now give a little protest time (~4-7days) for others, if noone objects i will merge it.

@JavaSaBr
Copy link
Copy Markdown
Contributor Author

JavaSaBr commented Jan 1, 2017

Cool :)

@shadowislord
Copy link
Copy Markdown
Member

So what are the advantages of using this allocator exactly? What's wrong with the regular one?

@JavaSaBr
Copy link
Copy Markdown
Contributor Author

JavaSaBr commented Jan 2, 2017

Java 9 and
http://www.java-gaming.org/index.php?topic=36524.0

@empirephoenix
Copy link
Copy Markdown
Contributor

The benefits are the one thing I'm certain about. Finally a way to efficiently manage memory manually in jme. No more directBuffer errors due to incompetent GC algorithms :) (Also it is not direct memory as far as I understand, so does not abide to it's limits, is swappable, ect.)

@Ali-RS
Copy link
Copy Markdown
Member

Ali-RS commented Jan 3, 2017

Also https://blog.lwjgl.org/memory-management-in-lwjgl-3/
It describes Memory management strategies.
Please take a look at Strategy4 description which is the current strategy for JME.

@JavaSaBr
Copy link
Copy Markdown
Contributor Author

I think all agree with this PR...

@empirephoenix empirephoenix merged commit 6493337 into jMonkeyEngine:master Jan 11, 2017
@empirephoenix
Copy link
Copy Markdown
Contributor

Merged it, thanks for the work :)

@JavaSaBr JavaSaBr deleted the added_lwjgl3_allocator branch January 15, 2017 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants