Skip to content

STRF-9112 Improve langJson javascript helper#234

Merged
junedkazi merged 1 commit intobigcommerce:masterfrom
jairo-bc:STRF-9112
May 7, 2021
Merged

STRF-9112 Improve langJson javascript helper#234
junedkazi merged 1 commit intobigcommerce:masterfrom
jairo-bc:STRF-9112

Conversation

@jairo-bc
Copy link
Copy Markdown
Contributor

@jairo-bc jairo-bc commented Apr 20, 2021

I believe Object.entries are being slow on GraalVM, so refactored it using simple for

cc @bigcommerce/storefront-team
Screenshot 2021-04-20 at 14 56 02

Comment thread lib/translator/filter.js
@@ -11,34 +11,27 @@
* @returns {Object.<string, string|Object>}
*/
function filterByKey(language, keyFilter) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

do we have existing tests to support the change ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes, we do

it('should return a cascaded translation object', done => {

it('should return a translation object filtered by key', done => {

Copy link
Copy Markdown
Contributor

@junedkazi junedkazi left a comment

Choose a reason for hiding this comment

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

Can you also attach some metrics to show the before and after improvements

@jairo-bc
Copy link
Copy Markdown
Contributor Author

@junedkazi I've added a screenshot of the current measurement on the second call.

Previously, I have those values:
Screenshot 2021-04-22 at 16 06 30
Screenshot 2021-04-22 at 16 00 04

@jairo-bc jairo-bc requested a review from junedkazi April 23, 2021 14:42
@junedkazi
Copy link
Copy Markdown
Contributor

You should also look at https://github.com/bigcommerce/paper/pull/218/files @mattolson had some performance improvements specially in the same file you are working on. I wonder if we should get Matt's changes rebased and merged first ? cc @jmwiese

@jairo-bc
Copy link
Copy Markdown
Contributor Author

@junedkazi We can test them in the graalvm, but those changes are mostly in languages transformation, that part was moved to storefront service.
I've also looked at https://github.com/bigcommerce/paper-handlebars/pull/113/files, eval is not the most costly operation here. Calling handlebars.template is https://github.com/bigcommerce/paper-handlebars/pull/113/files#diff-e727e4bdf3657fd1d798edcd6b099d6e092f8573cba266154583a746bba0f346R213

@jairo-bc
Copy link
Copy Markdown
Contributor Author

jairo-bc commented May 7, 2021

@junedkazi Could you please merge it, since I don't have rights for that?

@junedkazi junedkazi merged commit e75dac2 into bigcommerce:master May 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants