Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

FFM-11332 - Refactor evaluation logic to remove inefficiencies in the GetAttr(ibute) function #155

Merged
merged 3 commits into from
Apr 29, 2024
Merged

FFM-11332 - Refactor evaluation logic to remove inefficiencies in the GetAttr(ibute) function #155

merged 3 commits into from
Apr 29, 2024

Conversation

stephenmcconkey
Copy link
Contributor

@stephenmcconkey stephenmcconkey commented Apr 25, 2024 •
edited
Loading

What

  • Enhanced the getAttr function by implementing type-specific casting, which eliminates the repeated use of fmt.Sprintf.
  • Removed the usage of strings.ToLower within this function as both name and identifier are consistently returned in lowercase, and the associated map was never case-sensitive.
  • Added benchmark tests for the getAttr function to evaluate the performance improvements.
  • Removed redundant and inefficient logging that was not contributing to functionality or performance.

Why

Previous performance enhancements, while effective in certain aspects, led to unnecessary overuse of strings.ToLower and fmt.Sprintf. The frequent use of fmt.Sprintf was particularly redundant as most attributes are cast directly to a string value. Similarly, the application of strings.ToLower on every function call was excessive since the inputs are consistently lowercase. These changes are aimed at refining the efficiency of the getAttr function and reducing computational overhead.

Benchmarking after these changes:

/Users/stephenmcconkey/Library/Caches/JetBrains/GoLand2023.2/tmp/GoLand/___1gobench_github_com_harness_ff_golang_server_sdk_evaluation.test -test.v -test.paniconexit0 -test.bench . -test.run ^$
goos: darwin
goarch: arm64
pkg: github.com/harness/ff-golang-server-sdk/evaluation
BenchmarkEvaluateClause_NilClause
BenchmarkEvaluateClause_NilClause-10                 	530075331	         2.066 ns/op
BenchmarkEvaluateClause_EmptyOperator
BenchmarkEvaluateClause_EmptyOperator-10             	574456894	         2.067 ns/op
BenchmarkEvaluateClause_NilValues
BenchmarkEvaluateClause_NilValues-10                 	579981572	         2.087 ns/op
BenchmarkEvaluateClause_EmptyValues
BenchmarkEvaluateClause_EmptyValues-10               	576123748	         2.065 ns/op
BenchmarkEvaluateClause_WrongOperator
BenchmarkEvaluateClause_WrongOperator-10             	263199644	         4.555 ns/op
BenchmarkEvaluateClause_EmptyAttribute
BenchmarkEvaluateClause_EmptyAttribute-10            	376018725	         3.189 ns/op
BenchmarkEvaluateClause_MatchOperator
BenchmarkEvaluateClause_MatchOperator-10             	254281800	         4.702 ns/op
BenchmarkEvaluateClause_MatchOperatorError
BenchmarkEvaluateClause_MatchOperatorError-10        	255045268	         4.698 ns/op
BenchmarkEvaluateClause_InOperator
BenchmarkEvaluateClause_InOperator-10                	273828811	         4.452 ns/op
BenchmarkEvaluateClause_InOperatorNotFound
BenchmarkEvaluateClause_InOperatorNotFound-10        	269855254	         4.415 ns/op
BenchmarkEvaluateClause_EqualOperator
BenchmarkEvaluateClause_EqualOperator-10             	253553913	         4.737 ns/op
BenchmarkEvaluateClause_EqualSensitiveOperator
BenchmarkEvaluateClause_EqualSensitiveOperator-10    	253167048	         4.751 ns/op
BenchmarkEvaluateClause_GTOperator
BenchmarkEvaluateClause_GTOperator-10                	271229628	         4.440 ns/op
BenchmarkEvaluateClause_GTOperatorNegative
BenchmarkEvaluateClause_GTOperatorNegative-10        	268100358	         4.407 ns/op
BenchmarkEvaluateClause_StartsWithOperator
BenchmarkEvaluateClause_StartsWithOperator-10        	247472089	         4.746 ns/op
BenchmarkEvaluateClause_EndsWithOperator
BenchmarkEvaluateClause_EndsWithOperator-10          	254120905	         4.750 ns/op
BenchmarkEvaluateClause_ContainsOperator
BenchmarkEvaluateClause_ContainsOperator-10          	253459591	         4.700 ns/op
BenchmarkEvaluateClause_SegmentMatchOperator
BenchmarkEvaluateClause_SegmentMatchOperator-10      	377150246	         3.172 ns/op
BenchmarkGetAttrValueNilTarget
BenchmarkGetAttrValueNilTarget-10                    	578104182	         2.055 ns/op
BenchmarkGetAttrValueEmptyAttribute
BenchmarkGetAttrValueEmptyAttribute-10               	582490458	         2.055 ns/op
BenchmarkGetAttrValueIdentifier
BenchmarkGetAttrValueIdentifier-10                   	579464269	         2.055 ns/op
BenchmarkGetAttrValueName
BenchmarkGetAttrValueName-10                         	582064412	         2.058 ns/op
BenchmarkGetAttrValueUnmatched
BenchmarkGetAttrValueUnmatched-10                    	589708720	         2.028 ns/op
BenchmarkGetAttrValueStringAttr
BenchmarkGetAttrValueStringAttr-10                   	258044954	         4.687 ns/op
BenchmarkGetAttrValueIntegerAttr
BenchmarkGetAttrValueIntegerAttr-10                  	186729424	         6.425 ns/op
BenchmarkGetAttrValueFloatAttr
BenchmarkGetAttrValueFloatAttr-10                    	15980308	        73.92 ns/op
BenchmarkGetAttrValueBoolAttr
BenchmarkGetAttrValueBoolAttr-10                     	236984392	         5.057 ns/op
BenchmarkGetAttrValueBasicJSON
BenchmarkGetAttrValueBasicJSON-10                    	 6149366	       195.2 ns/op
BenchmarkGetAttrValueComplexJSON
BenchmarkGetAttrValueComplexJSON-10                  	 3165938	       389.8 ns/op
PASS

Benchmarking On Version 0.1.20

/Users/stephenmcconkey/Library/Caches/JetBrains/GoLand2023.2/tmp/GoLand/___1gobench_github_com_harness_ff_golang_server_sdk_evaluation.test -test.v -test.paniconexit0 -test.bench . -test.run ^$
goos: darwin
goarch: arm64
pkg: github.com/harness/ff-golang-server-sdk/evaluation
BenchmarkEvaluateClause_NilClause
BenchmarkEvaluateClause_NilClause-10                 	59983233	        19.97 ns/op
BenchmarkEvaluateClause_EmptyOperator
BenchmarkEvaluateClause_EmptyOperator-10             	57408598	        20.35 ns/op
BenchmarkEvaluateClause_NilValues
BenchmarkEvaluateClause_NilValues-10                 	58869822	        20.36 ns/op
BenchmarkEvaluateClause_EmptyValues
BenchmarkEvaluateClause_EmptyValues-10               	58767232	        20.11 ns/op
BenchmarkEvaluateClause_WrongOperator
BenchmarkEvaluateClause_WrongOperator-10             	93599180	        12.61 ns/op
BenchmarkEvaluateClause_EmptyAttribute
BenchmarkEvaluateClause_EmptyAttribute-10            	25940103	        45.15 ns/op
BenchmarkEvaluateClause_MatchOperator
BenchmarkEvaluateClause_MatchOperator-10             	91580625	        12.90 ns/op
BenchmarkEvaluateClause_MatchOperatorError
BenchmarkEvaluateClause_MatchOperatorError-10        	92633097	        12.85 ns/op
BenchmarkEvaluateClause_InOperator
BenchmarkEvaluateClause_InOperator-10                	95820814	        12.72 ns/op
BenchmarkEvaluateClause_InOperatorNotFound
BenchmarkEvaluateClause_InOperatorNotFound-10        	94503070	        12.60 ns/op
BenchmarkEvaluateClause_EqualOperator
BenchmarkEvaluateClause_EqualOperator-10             	93262434	        12.90 ns/op
BenchmarkEvaluateClause_EqualSensitiveOperator
BenchmarkEvaluateClause_EqualSensitiveOperator-10    	91337526	        12.93 ns/op
BenchmarkEvaluateClause_GTOperator
BenchmarkEvaluateClause_GTOperator-10                	95018150	        12.60 ns/op
BenchmarkEvaluateClause_GTOperatorNegative
BenchmarkEvaluateClause_GTOperatorNegative-10        	95914635	        12.58 ns/op
BenchmarkEvaluateClause_StartsWithOperator
BenchmarkEvaluateClause_StartsWithOperator-10        	93179457	        12.85 ns/op
BenchmarkEvaluateClause_EndsWithOperator
BenchmarkEvaluateClause_EndsWithOperator-10          	93337389	        12.95 ns/op
BenchmarkEvaluateClause_ContainsOperator
BenchmarkEvaluateClause_ContainsOperator-10          	92317754	        12.94 ns/op
BenchmarkEvaluateClause_SegmentMatchOperator
BenchmarkEvaluateClause_SegmentMatchOperator-10      	24913688	        48.18 ns/op
BenchmarkGetAttrValueNilTarget
BenchmarkGetAttrValueNilTarget-10                    	573551394	         2.065 ns/op
BenchmarkGetAttrValueEmptyAttribute
BenchmarkGetAttrValueEmptyAttribute-10               	339448593	         3.504 ns/op
BenchmarkGetAttrValueIdentifier
BenchmarkGetAttrValueIdentifier-10                   	100000000	        10.24 ns/op
BenchmarkGetAttrValueName
BenchmarkGetAttrValueName-10                         	208449168	         5.709 ns/op
BenchmarkGetAttrValueUnmatched
BenchmarkGetAttrValueUnmatched-10                    	121347568	         9.885 ns/op
BenchmarkGetAttrValueStringAttr
BenchmarkGetAttrValueStringAttr-10                   	27034789	        44.05 ns/op
BenchmarkGetAttrValueIntegerAttr
BenchmarkGetAttrValueIntegerAttr-10                  	27004117	        43.69 ns/op
BenchmarkGetAttrValueFloatAttr
BenchmarkGetAttrValueFloatAttr-10                    	13909501	        85.58 ns/op
BenchmarkGetAttrValueBoolAttr
BenchmarkGetAttrValueBoolAttr-10                     	28704643	        41.56 ns/op
BenchmarkGetAttrValueBasicJSON
BenchmarkGetAttrValueBasicJSON-10                    	 3484094	       342.9 ns/op
BenchmarkGetAttrValueComplexJSON
BenchmarkGetAttrValueComplexJSON-10                  	 1572980	       762.9 ns/op
PASS

Benchmarking On Version 0.1.18

/Users/stephenmcconkey/Library/Caches/JetBrains/GoLand2023.2/tmp/GoLand/___1gobench_github_com_harness_ff_golang_server_sdk_evaluation.test -test.v -test.paniconexit0 -test.bench . -test.run ^$
goos: darwin
goarch: arm64
pkg: github.com/harness/ff-golang-server-sdk/evaluation
BenchmarkEvaluateClause_NilClause
BenchmarkEvaluateClause_NilClause-10                 	546242251	         2.056 ns/op
BenchmarkEvaluateClause_EmptyOperator
BenchmarkEvaluateClause_EmptyOperator-10             	580436624	         2.061 ns/op
BenchmarkEvaluateClause_NilValues
BenchmarkEvaluateClause_NilValues-10                 	565921502	         2.120 ns/op
BenchmarkEvaluateClause_EmptyValues
BenchmarkEvaluateClause_EmptyValues-10               	566083796	         2.136 ns/op
BenchmarkEvaluateClause_WrongOperator
BenchmarkEvaluateClause_WrongOperator-10             	31626248	        37.93 ns/op
BenchmarkEvaluateClause_EmptyAttribute
BenchmarkEvaluateClause_EmptyAttribute-10            	76286340	        15.79 ns/op
BenchmarkEvaluateClause_MatchOperator
BenchmarkEvaluateClause_MatchOperator-10             	31306831	        38.27 ns/op
BenchmarkEvaluateClause_MatchOperatorError
BenchmarkEvaluateClause_MatchOperatorError-10        	31557147	        37.71 ns/op
BenchmarkEvaluateClause_InOperator
BenchmarkEvaluateClause_InOperator-10                	31973994	        38.57 ns/op
BenchmarkEvaluateClause_InOperatorNotFound
BenchmarkEvaluateClause_InOperatorNotFound-10        	31244575	        37.81 ns/op
BenchmarkEvaluateClause_EqualOperator
BenchmarkEvaluateClause_EqualOperator-10             	30933548	        37.94 ns/op
BenchmarkEvaluateClause_EqualSensitiveOperator
BenchmarkEvaluateClause_EqualSensitiveOperator-10    	30944118	        37.95 ns/op
BenchmarkEvaluateClause_GTOperator
BenchmarkEvaluateClause_GTOperator-10                	30949172	        37.60 ns/op
BenchmarkEvaluateClause_GTOperatorNegative
BenchmarkEvaluateClause_GTOperatorNegative-10        	31269542	        37.27 ns/op
BenchmarkEvaluateClause_StartsWithOperator
BenchmarkEvaluateClause_StartsWithOperator-10        	31621560	        37.85 ns/op
BenchmarkEvaluateClause_EndsWithOperator
BenchmarkEvaluateClause_EndsWithOperator-10          	31326684	        37.72 ns/op
BenchmarkEvaluateClause_ContainsOperator
BenchmarkEvaluateClause_ContainsOperator-10          	31334899	        37.72 ns/op
BenchmarkEvaluateClause_SegmentMatchOperator
BenchmarkEvaluateClause_SegmentMatchOperator-10      	75299824	        15.64 ns/op
BenchmarkGetAttrValueNilTarget
BenchmarkGetAttrValueNilTarget-10                    	579021799	         2.061 ns/op
BenchmarkGetAttrValueEmptyAttribute
BenchmarkGetAttrValueEmptyAttribute-10               	100000000	        11.79 ns/op
BenchmarkGetAttrValueIdentifier
BenchmarkGetAttrValueIdentifier-10                   	36185630	        33.17 ns/op
BenchmarkGetAttrValueName
BenchmarkGetAttrValueName-10                         	41398046	        28.71 ns/op
BenchmarkGetAttrValueUnmatched
BenchmarkGetAttrValueUnmatched-10                    	76750672	        15.40 ns/op
BenchmarkGetAttrValueStringAttr
BenchmarkGetAttrValueStringAttr-10                   	96950439	        11.92 ns/op
BenchmarkGetAttrValueIntegerAttr
BenchmarkGetAttrValueIntegerAttr-10                  	94883539	        12.42 ns/op
BenchmarkGetAttrValueFloatAttr
BenchmarkGetAttrValueFloatAttr-10                    	 8261598	       144.4 ns/op
BenchmarkGetAttrValueBoolAttr
BenchmarkGetAttrValueBoolAttr-10                     	100000000	        11.95 ns/op
BenchmarkGetAttrValueBasicJSON
BenchmarkGetAttrValueBasicJSON-10                    	 3456472	       345.0 ns/op
BenchmarkGetAttrValueComplexJSON
BenchmarkGetAttrValueComplexJSON-10                  	 1516618	       783.7 ns/op
PASS

Summary between this code (latest) and 0.1.18
Efficiency improvement for each benchmark test when comparing the latest version to version 0.1.18, shown in times faster than the original and in brackets where a positive percentage indicates a performance gain (improved efficiency):

  • EvaluateClause_NilClause: 1.00x (-0.49%)
  • EvaluateClause_EmptyOperator: 1.00x (-0.29%)
  • EvaluateClause_NilValues: 1.02x (1.56%)
  • EvaluateClause_EmptyValues: 1.03x (3.32%)
  • EvaluateClause_WrongOperator: 8.33x (87.99%)
  • EvaluateClause_EmptyAttribute: 4.95x (79.80%)
  • EvaluateClause_MatchOperator: 8.14x (87.71%)
  • EvaluateClause_MatchOperatorError: 8.03x (87.54%)
  • EvaluateClause_InOperator: 8.66x (88.46%)
  • EvaluateClause_InOperatorNotFound: 8.56x (88.32%)
  • EvaluateClause_EqualOperator: 8.01x (87.51%)
  • EvaluateClause_EqualSensitiveOperator: 7.99x (87.48%)
  • EvaluateClause_GTOperator: 8.47x (88.19%)
  • EvaluateClause_GTOperatorNegative: 8.46x (88.18%)
  • EvaluateClause_StartsWithOperator: 7.98x (87.46%)
  • EvaluateClause_EndsWithOperator: 7.94x (87.41%)
  • EvaluateClause_ContainsOperator: 8.03x (87.54%)
  • EvaluateClause_SegmentMatchOperator: 4.93x (79.72%)
  • GetAttrValueNilTarget: 1.00x (0.29%)
  • GetAttrValueEmptyAttribute: 5.74x (82.57%)
  • GetAttrValueIdentifier: 16.14x (93.80%)
  • GetAttrValueName: 13.95x (92.83%)
  • GetAttrValueUnmatched: 7.59x (86.83%)
  • GetAttrValueStringAttr: 2.54x (60.68%)
  • GetAttrValueIntegerAttr: 1.93x (48.27%)
  • GetAttrValueFloatAttr: 1.95x (48.81%)
  • GetAttrValueBoolAttr: 2.36x (57.68%)
  • GetAttrValueBasicJSON: 1.77x (43.42%)
  • GetAttrValueComplexJSON: 2.01x (50.26%)

These figures indicate substantial efficiency gains in the latest version across most benchmarks, with significant reductions in execution time for many operations, highlighting considerable performance optimisation.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@stephenmcconkey stephenmcconkey changed the title FFM-11332 - Refactor to remove inefficiencies in getAttr func FFM-11332 - Refactor evaluation logic to remove inefficiencies in the GetAttr(ibute) function Apr 26, 2024
davejohnston
davejohnston approved these changes Apr 26, 2024
View reviewed changes
Copy link
Contributor

@davejohnston davejohnston left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good.
I think worth getting Erdi and Andy Bs eyes on it as well.

@stephenmcconkey
Copy link
Contributor Author

Good to remove case sensitive check. Does not happen on the Java SDK
https://github.com/harness/ff-java-server-sdk/blob/main/src/main/java/io/harness/cf/client/api/Evaluator.java#L29

@stephenmcconkey stephenmcconkey merged commit 3cececf into main Apr 29, 2024
3 checks passed
@stephenmcconkey stephenmcconkey deleted the FFM-11332 branch April 29, 2024 10:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andybharness andybharness andybharness approved these changes

@davejohnston davejohnston davejohnston approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@stephenmcconkey @CLAassistant @davejohnston @andybharness