[fixed] Error parsing HTTP headers (2 Viewers)

[MrTechno]

Unit *and* performance testing? We are getting fancy

 

[morpheus_xx]

@mnmr, thanks, many valid points

I missed to change the redundant ToUpperInvariant() of keys, but your suggested solution sounds smarter to me. I will extend my UnitTest to do the parsing 1000 times, so we get a better measure. Then I'll try your way and check for logical correct result and also performance. I'm curious about results

I'm not sure if I should remove the exception for content-length/actual body size? I googled around and found some examples where exceptions are thrown. I also experienced myself faced with an proplem of IIS, it was canceling processing of pages, if the content-length was not matching the actual size.
So generally I would keep the exception in...

 

[mnmr]

I'm not sure if I should remove the exception for content-length/actual body size? I googled around and found some examples where exceptions are thrown. I also experienced myself faced with an proplem of IIS, it was canceling processing of pages, if the content-length was not matching the actual size.
So generally I would keep the exception in...

I agree, that is probably wisest. I would suggest to improve the error message though - perhaps by including the source of the offending packet, so that it's possible for people to disconnect or block whatever device is causing the problem.

It would also be useful with code to throttle logging, i.e. so identical messages only get written every so often, but that can be somewhat tricky to get right, since error logging often happens outside the normal flow of the code. A solution local to the class could reduce the number of throws (i.e. only throw the first time you see the error and then ignore any subsequent, identical errors for a while). A more generic solution would require you to keep state outside of the class (something like a dictionary of exception-type-with-message-and-origin to date/time-with-count) in order to know whether an exception should be logged. Given the current state of MP2, log throttling is probably not a priority ;-)

 

[morpheus_xx]

Ok, I made a quick test: using original code for 10.000 iterations, both LF+CRLF parsing. Test 2 contains the Dictionary with CI-comparer. All times in ms.

Curr   CI_Dict   FindIndexMultiPattern
423   627   916
373   604   926
405   657   990
376   612   933
409   631   979
376   599   927
393,67   621,67   945,17

Interesting is, that the other used dict causes an factor of 1.5 in runtime. So it reduce some "ToUpperInvariant" calls, but makes the code slower.

The 3rd test uses the FindIndexMultiPattern method. Runtime grows to nearly one second.

For me it means that the original code is the fastest way and works properly. I'm surprised about the impact of the changes. So thanks a lot that you shared your ideas, it helps to improve MP2...

 

[mnmr]

Interesting is, that the other used dict causes an factor of 1.5 in runtime. So it reduce some "ToUpperInvariant" calls, but makes the code slower.

If it's slower then it is doing something different (performance of string operations in .NET vary wildly). Try using StringComparer.OrdinalIgnoreCase instead.

The 3rd test uses the FindIndexMultiPattern method. Runtime grows to nearly one second.

I wasn't really writing the code with performance in mind. The LINQ methods Take/Skip/ToArray are not performance friendly (but very readable).

The idea with the method was actually not that the method itself should be faster than before, but that the code reading the network data could be improved (to avoid reading things byte-by-byte). I didn't include those changes, so if your test with the FindIndex method didn't use different code to read from the stream, then it would certainly be a lot slower.

If you have a ready-to-run project with the code and unit tests, I won't mind taking a look..

For me it means that the original code is the fastest way and works properly. I'm surprised about the impact of the changes. So thanks a lot that you shared your ideas, it helps to improve MP2...

I'll be glad to help where I can

 

[morpheus_xx]

I think I have "parked" the changes in a local branch. I can commit them this evening, so you can check each version

 

[morpheus_xx]

As follow up, here are the results using the OrdinalIgnoreCase comparer:

10T    CI Dict    OrdinalCI
379    642    387
384    600    349
371    629    348
407    600    381
388    630    359
411    593    378
390,00    615,67    367,00

I made a full new run to have comparable results. Interesting: the OrdinalIgnoreCase is slightly faster than the ToUpperInvariant() indexer

I think I will take this approach in combination with the old header parsing...

 

[mnmr]

That looks more like what I'd have expected.. If you'd like, I could still take a look at the other optimization, but since I'm not familiar with the code it'll be much easier if I have some existing unit tests to throw at any changes I make.

In any case, I really doubt this is a performance bottleneck in the system, so the main priority should be to have something that works

 

[morpheus_xx]

Changes pushed to https://github.com/MediaPortal/MediaPortal-2/commits/FIX_ParsingHttpHeaders_2
The unit test is included inside MP2-Client solution file.

Only change that is not commited is:
you need to add a loop "for(int i=0; i<10000; i++)") to line above
https://github.com/MediaPortal/Medi...f630#diff-e47255e65b377e7247652d928bf491cfR35
otherwise you won't get high enough values to compare when running the test from VS "test explorer"

 

[mnmr]

Ok, great - I'll see what I can come up with!