Skip to content

Task-2#11

Open
MarynaNogtieva wants to merge 12 commits intospajic:masterfrom
MarynaNogtieva:work-in-progress
Open

Task-2#11
MarynaNogtieva wants to merge 12 commits intospajic:masterfrom
MarynaNogtieva:work-in-progress

Conversation

@MarynaNogtieva
Copy link

No description provided.

Copy link
Owner

@spajic spajic left a comment

Choose a reason for hiding this comment

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

Very nice case-study! Thank you for your work!

On the other hand, `#each` method is called 25408 time and most of the time is spent in this method's children.

Based on the `GraphHtmlPrinter` report we observe same results,
where starting from line 100 there is a memory consuming code:
Copy link
Owner

Choose a reason for hiding this comment

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

time consuming


Just based on the above reports we can see that processing file was much faster after this refactoring.

#### Redults from RubyProf::FlatProfiler
Copy link
Owner

Choose a reason for hiding this comment

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

typo Redults -> Results

### Discovery №2
О вашей находке №2

For getting unique values we can use `Set` class.
Copy link
Owner

Choose a reason for hiding this comment

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

👍


and use `Set` during reading each line of file:
```
unique_browsers = Set.new([])
Copy link
Owner

Choose a reason for hiding this comment

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

You can also make use of SortedSet in this task.

b.include?(CHROME) #CHROME is a Constant
```

Also used `oj` gem to parse json based on this comparison gist I found https://gist.github.com/aishfenton/480059.
Copy link
Owner

Choose a reason for hiding this comment

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

👍


## Protection against performance regression

I added minitest to check that processing time of 1Mb file would not exceed 0.3 seconds
Copy link
Owner

Choose a reason for hiding this comment

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

👍

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