Skip to content
This repository has been archived by the owner on May 21, 2024. It is now read-only.

update: removed all the pointers and indirection #6

Closed
wants to merge 1 commit into from
Closed

update: removed all the pointers and indirection #6

wants to merge 1 commit into from

Conversation

traetox
Copy link

@traetox traetox commented Apr 24, 2018

I admittedly don't understand the purpose of all the pointers and indirection in the code, but i did a little light profiling and then rolled it in our own system and found that the extra pointers were causing more stress on the allocator and GC. I refactored the ragel system and some of the code to remove them and got between 10 and 60% speedups. If there is some other reason for keeping that indirection I would like to better understand it. I am also admittedly new to ragel and code generation, so if I botched some of the code changes let me know.

Thank you for the library, it was already fast, this is a little faster and more go-like.

benchmark                                                               old ns/op     new ns/op     delta
BenchmarkParse/[no]_empty_input__________________________________-4     270           233           -13.70%
BenchmarkParse/[no]_multiple_syslog_messages_on_multiple_lines___-4     519           322           -37.96%
BenchmarkParse/[no]_impossible_timestamp_________________________-4     1258          1065          -15.34%
BenchmarkParse/[no]_malformed_structured_data____________________-4     608           423           -30.43%
BenchmarkParse/[no]_with_duplicated_structured_data_id___________-4     1698          1397          -17.73%
BenchmarkParse/[ok]_minimal______________________________________-4     321           122           -61.99%
BenchmarkParse/[ok]_average_message______________________________-4     2588          2228          -13.91%
BenchmarkParse/[ok]_complicated_message__________________________-4     2176          1860          -14.52%
BenchmarkParse/[ok]_very_long_message____________________________-4     4501          4081          -9.33%
BenchmarkParse/[ok]_all_max_length_and_complete__________________-4     3514          3174          -9.68%
BenchmarkParse/[ok]_all_max_length_except_structured_data_and_mes-4     2115          1846          -12.72%
BenchmarkParse/[ok]_minimal_with_message_containing_newline______-4     384           157           -59.11%
BenchmarkParse/[ok]_w/o_procid,_w/o_structured_data,_with_message-4     1082          762           -29.57%
BenchmarkParse/[ok]_minimal_with_UTF-8_message___________________-4     608           370           -39.14%
BenchmarkParse/[ok]_with_structured_data_id,_w/o_structured_data_-4     1231          950           -22.83%
BenchmarkParse/[ok]_with_multiple_structured_data________________-4     1920          1603          -16.51%
BenchmarkParse/[ok]_with_escaped_backslash_within_structured_data-4     1667          1319          -20.88%
BenchmarkParse/[ok]_with_UTF-8_structured_data_param_value,_with_-4     1813          1596          -11.97%

@fntlnz fntlnz requested a review from leodido April 24, 2018 09:19
@leodido
Copy link
Collaborator

leodido commented Apr 24, 2018

Hi @traetox first of all thanks for your contribution.

I gavea quick look at it. Anyway after I'll look better at it.

But two considerations:

  • the part about output variable is probably correct
  • removing the pointers from the SyslogMessage fields is probably not what we want since we cannot represent the nil values of the RFC with the zero-values of golang types (except that for the version part)

Can you please reword your commit following the repository's convention?
Something like Update: Removing pointers from ... would be awesome.

@leodido leodido closed this Apr 24, 2018
@leodido leodido reopened this Apr 24, 2018
@traetox traetox changed the title removed all the pointers and indirection update: removed all the pointers and indirection Apr 24, 2018
@traetox
Copy link
Author

traetox commented Apr 24, 2018

I am going to rework the changes to incorporate an unexported flag that indicates whether or not the SyslogMessage is set. Accessor methods will then return the value and an ok which probably breaks your usage of the library. As such, I am going to close the PR.

@traetox traetox closed this Apr 24, 2018
@leodido leodido removed their request for review April 27, 2018 00:39
@leodido
Copy link
Collaborator

leodido commented Apr 27, 2018

Hey @traetox have a look at PR #7.

Now avoiding pointers & indirections as you suggested, but without chaning the interface at all (using an intermediate object and some flags).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants