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

Revert "perf(filter, yaml): replaced String with ComapctString" #985

Merged

Conversation

hitenkoku
Copy link
Collaborator

This reverts commit 096698a.

@hitenkoku hitenkoku self-assigned this Mar 26, 2023
@hitenkoku hitenkoku mentioned this pull request Mar 26, 2023
@codecov
Copy link

codecov bot commented Mar 26, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (60b14f0) 75.75% compared to head (7a446e2) 75.75%.

Additional details and impacted files
@@              Coverage Diff               @@
##           improve_speed     #985   +/-   ##
==============================================
  Coverage          75.75%   75.75%           
==============================================
  Files                 24       24           
  Lines              17090    17090           
==============================================
  Hits               12946    12946           
  Misses              4144     4144           
Impacted Files Coverage Δ
src/filter.rs 77.77% <100.00%> (ø)
src/yaml.rs 86.48% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@hitenkoku
Copy link
Collaborator Author

hitenkoku commented Mar 26, 2023

/ main #984 This PR
with-E Elapsed time: 00:04:10.513 Elapsed time: 00:04:08.585 Elapsed time: 00:04:08.705
without -E Elapsed time: 00:04:15.354 Elapsed time: 00:04:13.582 Elapsed time: 00:04:13.870

All of these results were run after the PC was rebooted.

PC Spec:

  • CPU AMD Ryzen 7 5700G with Radeon Graphics 3.80 GHz
  • RAM 16.0 GB
  • OS: Windows 11 Pro 22H2

In this environment, both #984 and this PR both have a certain effect and seem to take less time than the main one ......

Would you please check if this works in the Mac environment as well?

Copy link
Collaborator

@YamatoSecurity YamatoSecurity left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@fukusuket fukusuket left a comment

Choose a reason for hiding this comment

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

Sorry, my benchmark results are a bit inconsistent... :( (After restarting without starting any other applications...)
The result I got again was as follows. I think the difference in my environment is within the margin of error🤔

Version Option Elapsed time
main csv-timeline -E 00:06:38.389
#984 csv-timeline -E 00:06:34.896
This PR csv-timeline -E 00:06:39.268

Improvements have been confirmed in the Windows environment, so LGTM!!🚀

@YamatoSecurity
Copy link
Collaborator

2.3.2:
-EのEIDフィルタが有効の場合: 処理時間 00:20:42.163 メモリ使用: 12.5 GiB
無効の場合: 処理時間:00:25:46.253 メモリ使用: 12.5 GiB

#984:
-EのEIDフィルタが有効の場合: 処理時間 00:21:07.684 メモリ使用: 12.5 GiB
無効の場合: 処理時間: 00:26:08.211 メモリ使用: 12.2 GiB

This PR:
00:26:08.211 -> 00:25:44.313 メモリ: 12.2 GiB

まだ、無効にした場合しか確認できていませんが、CSVでは処理時間が2.3.2と同じぐらいですが、メモリ使用が300MB減っています。

@hitenkoku
Copy link
Collaborator Author

早速のご確認ありがとうございます。
他環境(Mac. Linux?)での改善も考えたらこちらのpull requestをimprove_speedにマージして、対応をしていこうとおもいます。

@hitenkoku hitenkoku merged commit 55ae66a into improve_speed Mar 28, 2023
@hitenkoku hitenkoku deleted the revert_096698ad31b3df77ceb4f59f3164229a6bae0d49 branch March 28, 2023 00:13
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.

3 participants