Uploaded image for project: 'MINA'
  1. MINA
  2. DIRMINA-210

Investigate removal of static methods in ByteBuffer

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Major
    • Resolution: Won't Fix
    • Affects Version/s: None
    • Fix Version/s: 2.0.8
    • Component/s: Core
    • Labels:
      None

      Description

      The removal of the static methods in ByteBuffer should be investigated.

      Since the ByteBufferAllocator exists, that interface could be used directly. For backwards compatibility ( and simple cases ), the static methods could be retained and use a supplied instance.

      Potentially, the MINA internals could all access buffers via the allocator, leaving only user-code to use the optional static methods (after statically supplying the allocator).

        Activity

        Hide
        niklas@trillian.se Niklas Therning added a comment -

        One way would be to let the user set an allocator on the IoServiceConfig used when binding and connecting. Acceptor/Connector/SocketIoProcessor would then use this allocator whenever allocating a buffer instead of using the static methods. The allocator could be propagated to the IoSessionConfig of new sessions to make it easily retrievable by user code whenever a session is available (e.g. in the IoHandler).

        Using this approach MINA internals would always use the allocator set by the user. Different allocators could be used for different server ports. It would be trivial to specify an allocator using Spring. And the user could easily get a hold of the configured allocator whenever a session is at hand.

        WDYT?

        Show
        niklas@trillian.se Niklas Therning added a comment - One way would be to let the user set an allocator on the IoServiceConfig used when binding and connecting. Acceptor/Connector/SocketIoProcessor would then use this allocator whenever allocating a buffer instead of using the static methods. The allocator could be propagated to the IoSessionConfig of new sessions to make it easily retrievable by user code whenever a session is available (e.g. in the IoHandler). Using this approach MINA internals would always use the allocator set by the user. Different allocators could be used for different server ports. It would be trivial to specify an allocator using Spring. And the user could easily get a hold of the configured allocator whenever a session is at hand. WDYT?
        Hide
        trustin Trustin Lee added a comment -

        The question here is how we can let user allocate buffers. Why I chose the use of static method was because it was very easy to use everywhere. Unlike NIO buffers, we have our own allocation strategy which is plugged in globally. The problem we're discussing arises when we want a multiple allocators for multiple services.

        Here's my idea about resolving this issue:

        1) Make MINA can have multiple allocators.
        2) Let user can specify the name or type of the allocator it wants to use. If not specified, the default one is used. We could also store the current allocator in thread local though I'm not sure using ThreadLocal will be OK under extreme situation.

        I don't like getting allocator by calling a method such as IoSession.getAllocator(), because the code will be not as simple as before.

        Show
        trustin Trustin Lee added a comment - The question here is how we can let user allocate buffers. Why I chose the use of static method was because it was very easy to use everywhere. Unlike NIO buffers, we have our own allocation strategy which is plugged in globally. The problem we're discussing arises when we want a multiple allocators for multiple services. Here's my idea about resolving this issue: 1) Make MINA can have multiple allocators. 2) Let user can specify the name or type of the allocator it wants to use. If not specified, the default one is used. We could also store the current allocator in thread local though I'm not sure using ThreadLocal will be OK under extreme situation. I don't like getting allocator by calling a method such as IoSession.getAllocator(), because the code will be not as simple as before.
        Hide
        niklas@trillian.se Niklas Therning added a comment -

        Trustin, could you please explain further how one could accomplish supporting multiple allocators in the same running MINA app and still use the static methods? Using different classloaders? Using ThreadLocals will be hard I think because of the asynchronous nature of MINA.

        What I proposed was a simple way of at least making the MINA internals use the allocator set by the user in the IoServiceConfig. It would then be up to the user to use the static methods and thus use the globally set allocator in his own code, or use the allocator which can be retrieved from the current session.

        Show
        niklas@trillian.se Niklas Therning added a comment - Trustin, could you please explain further how one could accomplish supporting multiple allocators in the same running MINA app and still use the static methods? Using different classloaders? Using ThreadLocals will be hard I think because of the asynchronous nature of MINA. What I proposed was a simple way of at least making the MINA internals use the allocator set by the user in the IoServiceConfig. It would then be up to the user to use the static methods and thus use the globally set allocator in his own code, or use the allocator which can be retrieved from the current session.
        Hide
        proyal peter royal added a comment -

        I think Niklas's approach has merit.. If we were to do this, ideally, we would get rid of the static methods. Thus, I don't think the ThreadLocal idea would be the best idea.

        The IoServiceConfig is the most logical place to put this.. And we can always have SimpleByteBuffer.allocate(), to create non-pooled buffers from user code. Heck, ByteBuffer.allocate() can even be retained, and it just returns a non-pooled heap buffer. As long as the core was changed to use the explicit allocator, users would be free to migrate their code at their own pace w/o breakage.

        Show
        proyal peter royal added a comment - I think Niklas's approach has merit.. If we were to do this, ideally, we would get rid of the static methods. Thus, I don't think the ThreadLocal idea would be the best idea. The IoServiceConfig is the most logical place to put this.. And we can always have SimpleByteBuffer.allocate(), to create non-pooled buffers from user code. Heck, ByteBuffer.allocate() can even be retained, and it just returns a non-pooled heap buffer. As long as the core was changed to use the explicit allocator, users would be free to migrate their code at their own pace w/o breakage.
        Hide
        trustin Trustin Lee added a comment -

        Yes, Niklas's approach has merit definitely. What we need to do is find out how to simplify the usage of the API. It would be nice if Niklas gives us some example code.

        I agree with Peter that IoServiceConfig is the place. Plus ByteBuffer.allocate() should be retained for backward compatibility and ease of use for simple servers which doesn't require extreme performance.

        Another question came out from my brain today is: do we really need a boolean parameter to choose the type of the buffer? I think it should go to the area of configuration (e.g. choosing different allocator). So this is another point of simplification IMHO.

        And here's more detail on my idea about retaining the static method:

        public static ByteBuffer allocate(int size) {
        Thread currentThread = Thread.currentThread();
        if( currentThread instanceof MinaWorker )

        { return ((MinaWorker) currentThread).getAssociatedAllocator().allocate(...); }

        else

        { return <simple allocation code here>; }

        }

        I'd not say this is easy to implement. Just want to know what people think about this approach.

        Show
        trustin Trustin Lee added a comment - Yes, Niklas's approach has merit definitely. What we need to do is find out how to simplify the usage of the API. It would be nice if Niklas gives us some example code. I agree with Peter that IoServiceConfig is the place. Plus ByteBuffer.allocate() should be retained for backward compatibility and ease of use for simple servers which doesn't require extreme performance. Another question came out from my brain today is: do we really need a boolean parameter to choose the type of the buffer? I think it should go to the area of configuration (e.g. choosing different allocator). So this is another point of simplification IMHO. And here's more detail on my idea about retaining the static method: public static ByteBuffer allocate(int size) { Thread currentThread = Thread.currentThread(); if( currentThread instanceof MinaWorker ) { return ((MinaWorker) currentThread).getAssociatedAllocator().allocate(...); } else { return <simple allocation code here>; } } I'd not say this is easy to implement. Just want to know what people think about this approach.
        Hide
        proyal peter royal added a comment -

        (Raw notes from IRC discussion)

        [09:49am] proyal: trustin, i like your idea about ByteBuffer.allocate, but i'd want to keep the existing way of doing it via statics, rather than having it check to the current thread.. ByteBuffer.allocate() would basically be a deprecated way.. IoSession.getAllocator().allocate() would be preferred (when the user wants pooling)
        [09:50am] luke_red5: 205 isnt critical for us, but I guess it would effect the spring api part
        [09:50am] trustin: proyal, yes it's simpler way for us. But IoSession.getAllocator().allocate() takes more time to type hmm..
        [09:50am] trustin: luke_red5, yes it's an API addition.
        [09:51am] luke_red5: i guess integration isnt that core to mina, at least i dont mind updating as we go
        [09:51am] vrm: I don't care much about memory pooling optimisations, I would hate to need to type IoSession.getAllocator().allocate()
        [09:51am] proyal: trustin: right, that's the downside, more typing..
        [09:51am] trustin: You're right. It's a good-to-have feature.
        [09:52am] proyal: i'm also 100% fine with leaving ByteBuffer allocation as-is
        [09:52am] trustin: Perhaps we could provide a method in IoHandlerAdapter.
        [09:52am] trustin: IoHandlerAdapter.allocate(session, size)?
        [09:52am] trustin: as a static method?
        [09:53am] proyal: trustin, heck, ByteBuffer.allocate( session, size) then..
        [09:53am] trustin: ahhh you're right, proyal
        [09:53am] trustin: that would be very nice.
        [09:53am] proyal: dunno if that's mixing the two interfaces together unnecessarily then..
        [09:53am] luke_red5: will it still be possible to allocate without a session
        [09:53am] proyal: but the general idea is fine
        [09:53am] doniv: that will create an unnecessary dep
        [09:53am] trustin: hmm right.
        [09:54am] trustin: though they are in the same package.
        [09:54am] doniv: starts getting ugly when u test bytebuffer
        [09:54am] trustin: It might be OK because these static methods are just a utility. hmm...
        [09:54am] trustin: yeah we'll need a mock object.
        [09:56am] proyal: luke_red5: yes, would be able to allocate w/o a session, just that there would be an Allocator per-IoHandler or such
        [09:56am] luke_red5: can someone tell me why the session is needed to be passed in. Will you set the allocator for the session?
        [09:56am] luke_red5: ok you answered before i asked
        [09:56am] proyal: so you could always instantiate your own allocator and allocate from that

        Show
        proyal peter royal added a comment - (Raw notes from IRC discussion) [09:49am] proyal: trustin, i like your idea about ByteBuffer.allocate, but i'd want to keep the existing way of doing it via statics, rather than having it check to the current thread.. ByteBuffer.allocate() would basically be a deprecated way.. IoSession.getAllocator().allocate() would be preferred (when the user wants pooling) [09:50am] luke_red5: 205 isnt critical for us, but I guess it would effect the spring api part [09:50am] trustin: proyal, yes it's simpler way for us. But IoSession.getAllocator().allocate() takes more time to type hmm.. [09:50am] trustin: luke_red5, yes it's an API addition. [09:51am] luke_red5: i guess integration isnt that core to mina, at least i dont mind updating as we go [09:51am] vrm: I don't care much about memory pooling optimisations, I would hate to need to type IoSession.getAllocator().allocate() [09:51am] proyal: trustin: right, that's the downside, more typing.. [09:51am] trustin: You're right. It's a good-to-have feature. [09:52am] proyal: i'm also 100% fine with leaving ByteBuffer allocation as-is [09:52am] trustin: Perhaps we could provide a method in IoHandlerAdapter. [09:52am] trustin: IoHandlerAdapter.allocate(session, size)? [09:52am] trustin: as a static method? [09:53am] proyal: trustin, heck, ByteBuffer.allocate( session, size) then.. [09:53am] trustin: ahhh you're right, proyal [09:53am] trustin: that would be very nice. [09:53am] proyal: dunno if that's mixing the two interfaces together unnecessarily then.. [09:53am] luke_red5: will it still be possible to allocate without a session [09:53am] proyal: but the general idea is fine [09:53am] doniv: that will create an unnecessary dep [09:53am] trustin: hmm right. [09:54am] trustin: though they are in the same package. [09:54am] doniv: starts getting ugly when u test bytebuffer [09:54am] trustin: It might be OK because these static methods are just a utility. hmm... [09:54am] trustin: yeah we'll need a mock object. [09:56am] proyal: luke_red5: yes, would be able to allocate w/o a session, just that there would be an Allocator per-IoHandler or such [09:56am] luke_red5: can someone tell me why the session is needed to be passed in. Will you set the allocator for the session? [09:56am] luke_red5: ok you answered before i asked [09:56am] proyal: so you could always instantiate your own allocator and allocate from that
        Hide
        elecharny Emmanuel Lecharny added a comment -

        Postponed to the next version

        Show
        elecharny Emmanuel Lecharny added a comment - Postponed to the next version
        Hide
        elecharny Emmanuel Lecharny added a comment -

        Static allocator in ByteBuffer or in IoBuffer have been around for 8 years now. Nobody complained about it, and I don't see how exposing the allocator can bring a lot of added value at this point.

        For MINA 2, at least, I think it would be insane to change that. For MINA 3, this can be discussed.

        Show
        elecharny Emmanuel Lecharny added a comment - Static allocator in ByteBuffer or in IoBuffer have been around for 8 years now. Nobody complained about it, and I don't see how exposing the allocator can bring a lot of added value at this point. For MINA 2, at least, I think it would be insane to change that. For MINA 3, this can be discussed.

          People

          • Assignee:
            proyal peter royal
            Reporter:
            proyal peter royal
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development