Refactor/transactor status by miguelsorianod · Pull Request #26 · 3scale/apisonator

miguelsorianod · 2018-05-02T14:35:52Z

I have refactored some parts of the transactor/status class.

Also I have removed some unreferenced methods/code from the class. I removed the options on the to_xml method named 'exclude_application' and 'exclude_user' because are not referenced/set anywhere on the code and seem to be legacy options (it seems were added in commit 3424475). Maybe @unleashed / @mikz that have been longer with the codebase can confirm this.

Thank you.

The application_plan_name method in transactor/status is not called from any other part of the codebase.

eguzki

LGTM

However, building xml's using a library like Nokogiri::XML::Builder seems more appropriate. It is always less error prone than using string concatenations.

mikz · 2018-05-02T18:57:59Z

        end

+        def add_authorize_section(xml)
+          xml << '<authorized>'


This is not frozen and will allocate plenty of strings.

I have updated the code to freeze the strings.

Thank you.

miguelsorianod · 2018-05-03T08:26:47Z

Regarding the use of Nokogiri::XML::Builder, I agree with you @eguzki

The idea is to move this code out of transactor/Status.rb in another PR, in order to decouple the data representation from the model, along with trying to generalize the data representation to be compatible with other formats (like json).

unleashed · 2018-05-03T11:58:11Z

+        def add_authorize_section(xml)
+          xml << '<authorized>'.freeze
+          if authorized?
+            xml << 'true'.freeze << '</authorized>'.freeze


you can make this perform better with a single xml << '<authorized>true</authorized>'.freeze

unleashed · 2018-05-03T11:58:42Z

+            xml << 'true'.freeze << '</authorized>'.freeze
+          else
+            xml << 'false'.freeze << '</authorized>'.freeze
+            xml << '<reason>'.freeze


here you can do xml << '<authorized>false</authorized><reason>'.freeze

the same principle applies to other places in the added methods

👍 . Done. In some other places I left it the way it is for readability

unleashed · 2018-05-04T07:15:31Z

+          redirect_uri = application.redirect_url
+          xml << "<application><id>".freeze << "#{application.id}" << "</id>".freeze
+          xml << "<key>".freeze << "#{application.keys.first}" << "</key>".freeze
+          xml << "<#{@redirect_uri_field}>#{redirect_uri}</#{@redirect_uri_field}>"


you can join here literals in a single string: "</id><key>", "</key><" as it will avoid a couple of string concatenation ops.

unleashed · 2018-05-04T07:16:30Z

+        end
+
+        def add_user_section(xml)
+          if !@user.nil?


have you considered moving the check to the caller?

unleashed

Check my comment, it's the only remaining thing.

unleashed · 2018-05-10T16:20:48Z

+          xml << "<application><id>".freeze << "#{application.id}" << "</id><key>".freeze
+          xml << "#{application.keys.first}" << "</key><".freeze
+          xml << "#{@redirect_uri_field}>#{redirect_uri}</#{@redirect_uri_field}"
+          xml << '></application>'.freeze


here you could move the "</key><" literal to prefix and the '></application>' literal to suffix the line with the redirects to avoid two << operations.

If I do that then this literals will not be frozen even if I put a .freeze, which we were trying to do for the literal parts as mikz commented.

@miguelsorianod but there is no good reason to have extra frozen string when you are allocating it moments before anyway

RubyVM::InstructionSequence.compile(<<'RUBY').disasm xml << "#{@redirect_uri_field}>#{redirect_uri}</#{@redirect_uri_field}"; xml << '></application>'.freeze RUBY

== disasm: #<ISeq:<compiled>@<compiled>>================================ 0000 trace 1 ( 1) 0002 putself 0003 opt_send_without_block <callinfo!mid:xml, argc:0, FCALL|VCALL|ARGS_SIMPLE>, <callcache> 0006 getinstancevariable :@redirect_uri_field, <is:0> 0009 tostring 0010 putobject ">" 0012 putself 0013 opt_send_without_block <callinfo!mid:redirect_uri, argc:0, FCALL|VCALL|ARGS_SIMPLE>, <callcache> 0016 tostring 0017 putobject "</" 0019 getinstancevariable :@redirect_uri_field, <is:0> 0022 tostring 0023 concatstrings 5 0025 opt_ltlt <callinfo!mid:<<, argc:1, ARGS_SIMPLE>, <callcache> 0028 pop 0029 trace 1 ( 2) 0031 putself 0032 opt_send_without_block <callinfo!mid:xml, argc:0, FCALL|VCALL|ARGS_SIMPLE>, <callcache> 0035 opt_str_freeze "></application>" 0037 opt_ltlt <callinfo!mid:<<, argc:1, ARGS_SIMPLE>, <callcache> 0040 leave

versus

RubyVM::InstructionSequence.compile(<<'RUBY').disasm xml << "#{@redirect_uri_field}>#{redirect_uri}</#{@redirect_uri_field}></application>" RUBY

== disasm: #<ISeq:<compiled>@<compiled>>================================ 0000 trace 1 ( 1) 0002 putself 0003 opt_send_without_block <callinfo!mid:xml, argc:0, FCALL|VCALL|ARGS_SIMPLE>, <callcache> 0006 getinstancevariable :@redirect_uri_field, <is:0> 0009 tostring 0010 putobject ">" 0012 putself 0013 opt_send_without_block <callinfo!mid:redirect_uri, argc:0, FCALL|VCALL|ARGS_SIMPLE>, <callcache> 0016 tostring 0017 putobject "</" 0019 getinstancevariable :@redirect_uri_field, <is:0> 0022 tostring 0023 putobject "></application>" 0025 concatstrings 6 0027 opt_ltlt <callinfo!mid:<<, argc:1, ARGS_SIMPLE>, <callcache> 0030 leave

in other words: there is one allocation already going on there with the interpolation, and freezing and adding the frozen strings adds two extra operations, whereas those strings could already be part of the allocated string in the first place.

I understand that an allocation is less costly that an allocation + a concatenation in that specific case. My doubt was about what happened if that same literal would appear in other places of the code.
The solution then would be to freeze the '>' part in the other part of the code. It would not make sense to freeze it in this specific section of the code because anyway you will be doing an allocation for the redirect part. I'll change it now, thank you!

The thing is what are you willing to optimize for. If you have the same literals sprinkled elsewhere you can make all of them frozen, only some, or none. With strings as interpreted in MRI, using .freeze makes the effect of interning them, which is to say they become similar to static, immutable constants. In that case, when the interpreter finds references to the same string (and also with .freeze*), it will refer to the interned one to save on unneeded allocations and extra copies.

But of course, having a few copies of the same string (or more accurately, substring) to save in execution speed is usually a good tradeoff. So, in some cases you can intern them, but it only makes sense if you actually save the allocation, and if that's not the case, it's worse if it also adds an extra cost in execution time.

*: Ruby 2.3+ have flags for making the String#freeze behaviour the default with literals.

I understand that an allocation is less costly that an allocation + a concatenation in that specific case. My doubt was about what happened if that same literal would appear in other places of the code.
The solution then would be to freeze the '>' part in the other part of the code. It would not make sense to freeze it in this specific section of the code because anyway you will be doing an allocation for the redirect part. I have changed it and pushed the changes already, thank you!

This is 93% the same comment as above! I want my 10s back!!

mikz · 2018-05-11T07:25:47Z

        def add_application_section(xml)
          redirect_uri = application.redirect_url
-          xml << '<application>' \
+          xml << "<application>" \


I'd say this was correctly using single quotes. There is no interpolation within them so it signals to the reader: don't worry, this is the actual string, no interpolation there.

👍 changed.

…tions The to_xml method was checking for exclude_application and exclude_user options but there are not any references to this options in the code. We remove them.

mikz

👍

unleashed · 2018-05-11T08:47:33Z

bors r=@eguzki,@davidor,@mikz,@unleashed

@unleashed

26: Refactor/transactor status r=eguzki,davidor,mikz,unleashed a=miguelsorianod I have refactored some parts of the transactor/status class. Also I have removed some unreferenced methods/code from the class. I removed the options on the to_xml method named 'exclude_application' and 'exclude_user' because are not referenced/set anywhere on the code and seem to be legacy options (it seems were added in commit 3424475). Maybe @unleashed / @mikz that have been longer with the codebase can confirm this. Thank you. Co-authored-by: Miguel Soriano <msoriano@redhat.com>

bors · 2018-05-11T08:49:40Z

Build failed

ci/circleci

unleashed · 2018-05-11T09:04:45Z

bors r=@eguzki,@davidor,@mikz,@unleashed

@unleashed

26: Refactor/transactor status r=eguzki,davidor,mikz,unleashed a=miguelsorianod I have refactored some parts of the transactor/status class. Also I have removed some unreferenced methods/code from the class. I removed the options on the to_xml method named 'exclude_application' and 'exclude_user' because are not referenced/set anywhere on the code and seem to be legacy options (it seems were added in commit 3424475). Maybe @unleashed / @mikz that have been longer with the codebase can confirm this. Thank you. Co-authored-by: Miguel Soriano <msoriano@redhat.com>

bors · 2018-05-11T09:06:26Z

Build failed

ci/circleci

unleashed · 2018-05-11T09:11:13Z

last try, otherwise it is clear that some powerful, higher order entity does not want this merged, and if so better not to annoy them (a la tercera va la vencida)

bors r=@eguzki,@davidor,@mikz,@unleashed

@unleashed

26: Refactor/transactor status r=eguzki,davidor,mikz,unleashed a=miguelsorianod I have refactored some parts of the transactor/status class. Also I have removed some unreferenced methods/code from the class. I removed the options on the to_xml method named 'exclude_application' and 'exclude_user' because are not referenced/set anywhere on the code and seem to be legacy options (it seems were added in commit 3424475). Maybe @unleashed / @mikz that have been longer with the codebase can confirm this. Thank you. Co-authored-by: Miguel Soriano <msoriano@redhat.com>

bors · 2018-05-11T09:14:35Z

Build succeeded

ci/circleci

miguelsorianod added 2 commits May 2, 2018 16:05

transactor/status: Refactor. Extract methods from to_xml

7ad797f

transactor/status: Remove unreferenced application_plan_name method

b8a510d

The application_plan_name method in transactor/status is not called from any other part of the codebase.

miguelsorianod requested review from davidor, eguzki, mikz and unleashed May 2, 2018 14:35

davidor approved these changes May 2, 2018

View reviewed changes

eguzki reviewed May 2, 2018

View reviewed changes

mikz reviewed May 2, 2018

View reviewed changes

miguelsorianod force-pushed the refactor/transactor_status branch from 1a90d06 to 78b3bd4 Compare May 3, 2018 08:20

miguelsorianod requested a review from mikz May 3, 2018 08:22

miguelsorianod force-pushed the refactor/transactor_status branch from 78b3bd4 to 669f5eb Compare May 3, 2018 08:38

miguelsorianod requested a review from eguzki May 3, 2018 08:39

miguelsorianod force-pushed the refactor/transactor_status branch from 669f5eb to 18f9a08 Compare May 3, 2018 08:44

eguzki approved these changes May 3, 2018

View reviewed changes

unleashed reviewed May 3, 2018

View reviewed changes

miguelsorianod force-pushed the refactor/transactor_status branch from 18f9a08 to f1b0a91 Compare May 3, 2018 12:09

miguelsorianod requested a review from unleashed May 3, 2018 12:10

miguelsorianod force-pushed the refactor/transactor_status branch from f1b0a91 to 2bedf8b Compare May 3, 2018 17:02

unleashed reviewed May 4, 2018

View reviewed changes

miguelsorianod force-pushed the refactor/transactor_status branch from 2bedf8b to 2e77a83 Compare May 4, 2018 07:43

unleashed reviewed May 10, 2018

View reviewed changes

miguelsorianod force-pushed the refactor/transactor_status branch from 2e77a83 to 6718e90 Compare May 11, 2018 07:22

miguelsorianod requested a review from unleashed May 11, 2018 07:23

mikz reviewed May 11, 2018

View reviewed changes

transactor/status: Remove unused exclude_application, exclude_user op…

97ae3ab

…tions The to_xml method was checking for exclude_application and exclude_user options but there are not any references to this options in the code. We remove them.

miguelsorianod force-pushed the refactor/transactor_status branch from 6718e90 to 97ae3ab Compare May 11, 2018 07:27

miguelsorianod requested a review from mikz May 11, 2018 07:28

mikz approved these changes May 11, 2018

View reviewed changes

unleashed approved these changes May 11, 2018

View reviewed changes

bors Bot merged commit 97ae3ab into master May 11, 2018

bors Bot deleted the refactor/transactor_status branch May 11, 2018 09:14

eguzki mentioned this pull request May 11, 2018

Update nokogiri to 1.8.2 #28

Merged

Conversation

miguelsorianod commented May 2, 2018

Uh oh!

eguzki left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miguelsorianod commented May 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

unleashed May 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

unleashed left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikz May 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

unleashed May 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikz left a comment

Choose a reason for hiding this comment

Uh oh!

unleashed commented May 11, 2018

Uh oh!

bors Bot commented May 11, 2018

Build failed

Uh oh!

unleashed commented May 11, 2018

Uh oh!

bors Bot commented May 11, 2018

Build failed

Uh oh!

unleashed commented May 11, 2018

Uh oh!

bors Bot commented May 11, 2018

Build succeeded

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

unleashed May 4, 2018 •

edited

Loading

mikz May 10, 2018 •

edited

Loading

unleashed May 10, 2018 •

edited

Loading