Change in openflowjava[master]: Migrate OFFrameDecoder to LengthFieldBasedDecoder v2


Anton Ivanov
 

Removing jenkings-releng/Adding openflowjava-dev.

OK.

I will double check the error/exception handling code in LengthFieldBasedDecoder vs OFFrameDecoder.

With the zero copy in place they are now equivalent performance-wize so the sole difference should be if there is any extra error checking in the stock LengthFieldBasedDecoder.

By the way, it looks like jenkins is failing on a completely unrelated item - log4j/integration towards the end of the test suite.

A.

On 08/01/15 12:48, Michal Polkoráb wrote:
I think yes. But I believe, that until we gain some performance boost, the change in OFFrameDecoder is unnecessary - as the LengthFieldBasedDecoder only hides what is written in OFFrameDecoder.

Michal
________________________________________
From: Anton Ivanov <anton.ivanov@...>
Sent: 08 January 2015 12:45
To: Michal Polkoráb
Cc: jenkins-releng
Subject: Re: Change in openflowjava[master]: Migrate OFFrameDecoder to LengthFieldBasedDecoder v2

Thanks,

Noted.

If I understand the code correctly this can be achieved by overriding
extractFrame() in a LengthFieldBasedDecoder descendant instead of
rewriting the whole decode.

I have an updated version under test and I will submit a v3 which is
zero copy shortly.

A.

On 08/01/15 10:50, Gerrit Code Review wrote:
From Michal Polkorab <michal.polkorab@...>:

Michal Polkorab has posted comments on this change.

Change subject: Migrate OFFrameDecoder to LengthFieldBasedDecoder v2
......................................................................


Patch Set 1: Code-Review-1

Hi Anton,

OFFrameDecoder is implemented as is intentionally. We knew about LengthFieldBasedDecoder, but we didn't use it because of memory copy it performs. LengthFieldBasedDecoder allocates new Buffer and copies message (frame) data into this buffer for each message. This is why we didn't use this implementation and why we used slicing (zero copy / scatter buffer) inside OFFrameDecoder.
MichalPolkoráb
Software Developer

Mlynské Nivy 56 / 821 05 Bratislava / Slovakia
+421 918 378 907 / michal.polkorab@...
reception: +421 2 206 65 111 / www.pantheon.sk
[logo]


Michal Polkorab
 

Yes, I see. The problem is in controller features: No feature named 'odl-mdsal-common' with version '1.2.0-SNAPSHOT' available
Those have been probably updated. I will try to investigate what happened and fix it. After that I can retrigger your job.

Michal
________________________________________
From: Anton Ivanov <anton.ivanov@...>
Sent: 08 January 2015 14:21
To: Michal Polkoráb; openflowjava-dev@...
Subject: Re: Change in openflowjava[master]: Migrate OFFrameDecoder to LengthFieldBasedDecoder v2

Removing jenkings-releng/Adding openflowjava-dev.

OK.

I will double check the error/exception handling code in
LengthFieldBasedDecoder vs OFFrameDecoder.

With the zero copy in place they are now equivalent performance-wize so
the sole difference should be if there is any extra error checking in
the stock LengthFieldBasedDecoder.

By the way, it looks like jenkins is failing on a completely unrelated
item - log4j/integration towards the end of the test suite.

A.

On 08/01/15 12:48, Michal Polkoráb wrote:
I think yes. But I believe, that until we gain some performance boost, the change in OFFrameDecoder is unnecessary - as the LengthFieldBasedDecoder only hides what is written in OFFrameDecoder.

Michal
________________________________________
From: Anton Ivanov <anton.ivanov@...>
Sent: 08 January 2015 12:45
To: Michal Polkoráb
Cc: jenkins-releng
Subject: Re: Change in openflowjava[master]: Migrate OFFrameDecoder to LengthFieldBasedDecoder v2

Thanks,

Noted.

If I understand the code correctly this can be achieved by overriding
extractFrame() in a LengthFieldBasedDecoder descendant instead of
rewriting the whole decode.

I have an updated version under test and I will submit a v3 which is
zero copy shortly.

A.

On 08/01/15 10:50, Gerrit Code Review wrote:
From Michal Polkorab <michal.polkorab@...>:

Michal Polkorab has posted comments on this change.

Change subject: Migrate OFFrameDecoder to LengthFieldBasedDecoder v2
......................................................................


Patch Set 1: Code-Review-1

Hi Anton,

OFFrameDecoder is implemented as is intentionally. We knew about LengthFieldBasedDecoder, but we didn't use it because of memory copy it performs. LengthFieldBasedDecoder allocates new Buffer and copies message (frame) data into this buffer for each message. This is why we didn't use this implementation and why we used slicing (zero copy / scatter buffer) inside OFFrameDecoder.
MichalPolkoráb
Software Developer

Mlynské Nivy 56 / 821 05 Bratislava / Slovakia
+421 918 378 907 / michal.polkorab@...
reception: +421 2 206 65 111 / www.pantheon.sk
[logo]
MichalPolkoráb
Software Developer

Mlynské Nivy 56 / 821 05 Bratislava / Slovakia
+421 918 378 907 / michal.polkorab@...
reception: +421 2 206 65 111 / www.pantheon.sk
[logo]


Anton Ivanov
 

On 08/01/15 13:33, Michal Polkoráb wrote:
Yes, I see. The problem is in controller features: No feature named 'odl-mdsal-common' with version '1.2.0-SNAPSHOT' available
Those have been probably updated. I will try to investigate what happened and fix it. After that I can retrigger your job.
Thanks.

Otherwise, the stock netty LengthFieldBasedDecoder also contains a couple of checks for corrupted frames which are missing in the OFFrameDecoder.

These can catch if someone feeds ODL a frame with invalid length. However, that check is actually too weak to catch all types of frames which are corrupted at framing level.

IMHO, In either case (my version or stock) there should be an extra check to ensure that the length read off the stream is sane before it is being used.

A.


Michal
________________________________________
From: Anton Ivanov <anton.ivanov@...>
Sent: 08 January 2015 14:21
To: Michal Polkoráb; openflowjava-dev@...
Subject: Re: Change in openflowjava[master]: Migrate OFFrameDecoder to LengthFieldBasedDecoder v2

Removing jenkings-releng/Adding openflowjava-dev.

OK.

I will double check the error/exception handling code in
LengthFieldBasedDecoder vs OFFrameDecoder.

With the zero copy in place they are now equivalent performance-wize so
the sole difference should be if there is any extra error checking in
the stock LengthFieldBasedDecoder.

By the way, it looks like jenkins is failing on a completely unrelated
item - log4j/integration towards the end of the test suite.

A.

On 08/01/15 12:48, Michal Polkoráb wrote:
I think yes. But I believe, that until we gain some performance boost, the change in OFFrameDecoder is unnecessary - as the LengthFieldBasedDecoder only hides what is written in OFFrameDecoder.

Michal
________________________________________
From: Anton Ivanov <anton.ivanov@...>
Sent: 08 January 2015 12:45
To: Michal Polkoráb
Cc: jenkins-releng
Subject: Re: Change in openflowjava[master]: Migrate OFFrameDecoder to LengthFieldBasedDecoder v2

Thanks,

Noted.

If I understand the code correctly this can be achieved by overriding
extractFrame() in a LengthFieldBasedDecoder descendant instead of
rewriting the whole decode.

I have an updated version under test and I will submit a v3 which is
zero copy shortly.

A.

On 08/01/15 10:50, Gerrit Code Review wrote:
From Michal Polkorab <michal.polkorab@...>:

Michal Polkorab has posted comments on this change.

Change subject: Migrate OFFrameDecoder to LengthFieldBasedDecoder v2
......................................................................


Patch Set 1: Code-Review-1

Hi Anton,

OFFrameDecoder is implemented as is intentionally. We knew about LengthFieldBasedDecoder, but we didn't use it because of memory copy it performs. LengthFieldBasedDecoder allocates new Buffer and copies message (frame) data into this buffer for each message. This is why we didn't use this implementation and why we used slicing (zero copy / scatter buffer) inside OFFrameDecoder.
MichalPolkoráb
Software Developer

Mlynské Nivy 56 / 821 05 Bratislava / Slovakia
+421 918 378 907 / michal.polkorab@...
reception: +421 2 206 65 111 / www.pantheon.sk
[logo]
MichalPolkoráb
Software Developer

Mlynské Nivy 56 / 821 05 Bratislava / Slovakia
+421 918 378 907 / michal.polkorab@...
reception: +421 2 206 65 111 / www.pantheon.sk
[logo]