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.

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

Fix memory consumption bug #702

Merged
merged 4 commits into from Mar 16, 2018
Merged

Fix memory consumption bug #702

merged 4 commits into from Mar 16, 2018

Conversation

funkyboy
Copy link
Contributor

Fix #691

Source/ARTLog.m Outdated
@@ -109,7 +109,7 @@ - (instancetype)init {
}

- (instancetype)initCapturingOutput:(BOOL)capturing {
return [self initCapturingOutput:true historyLines:100];
return [self initCapturingOutput:true historyLines:50];
Copy link
Contributor

Choose a reason for hiding this comment

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

The history is sent to Sentry when an exception occurs. I think 50 is too low. 100 lines would give us more elements to diagnose an issue. Does this change improve significantly the performance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will measure and let you know.

Copy link
Contributor

@ricardopereira ricardopereira left a comment

Choose a reason for hiding this comment

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

It looks good but I leaved a small comment.

@ricardopereira
Copy link
Contributor

@funkyboy
Copy link
Contributor Author

@ricardopereira I did some profiling. 50 or 100 history items seems not a big deal at all.
Adding some screenshots here for future reference.

50
100

@ricardopereira
Copy link
Contributor

@funkyboy Ok, thanks for checking!

@funkyboy funkyboy merged commit 2b9e6a7 into master Mar 16, 2018
@funkyboy funkyboy deleted the fix-memory-consumption-bug branch March 16, 2018 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants