Skip to content

opt: redesign the truncate effect logic to reduce memory cost in text mode #718 #775

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
Merged
merged 1 commit into from
May 13, 2025
Merged

opt: redesign the truncate effect logic to reduce memory cost in text mode #718 #775

merged 1 commit into from
May 13, 2025

Conversation

chilli13
Copy link
Contributor

@chilli13 chilli13 commented May 8, 2025

@dosubot dosubot bot added the enhancement New feature or request label May 8, 2025
@chilli13
Copy link
Contributor Author

chilli13 commented May 8, 2025

@yuweizzz #731 can not solve issue memory cost too much when deal big files, as ew.writeEvent(e) keeps writing all data to ew.payload in function func (ew *eventWorker) Run(), this pull try to limit payload len before ew.writeEvent(e), please help to review, thks

yuweizzz
yuweizzz reviewed May 8, 2025
View reviewed changes
pkg/event_processor/iworker.go Outdated
// 输出包
if ew.tickerCount > MaxTickerCount {
if truncateFlag || ew.tickerCount > MaxTickerCount {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is better to move this part into func (ew *eventWorker) writeEvent(e event.IEventStruct).

@chilli13 chilli13 force-pushed the opt/truncate branch 2 times, most recently from 3638bb5 to 559c848 Compare May 8, 2025 06:48
cfc4n
cfc4n requested changes May 10, 2025
View reviewed changes
pkg/event_processor/iworker.go Outdated
if ew.status != ProcessStateInit {
_ = ew.writeToChan("write events failed, unknow eventWorker status")
return
return false
}
ew.payload.Write(e.Payload())
Copy link
Member

Choose a reason for hiding this comment

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

为什么不放在这里获取tzise，而是通过外部传递呢？

建议还原所有代码，之后只保留一下更改。

tsize := int(ew.processor.truncateSize)
//terminal write when reach the truncate size
	if tsize > 0 && ew.payload.Len() >= tsize {
		ew.payload.Truncate(tsize)
		_ = ew.writeToChan(fmt.Sprintf("Events truncated, size: %d bytes\n", tsize))
		return true
	}
	return false

cfc4n
cfc4n requested changes May 12, 2025
View reviewed changes
Copy link
Member

@cfc4n cfc4n left a comment

Choose a reason for hiding this comment

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

Do you confirm that clearing the payload, moving from parserEvents to writeEvent, can optimize memory usage?

@chilli13
Copy link
Contributor Author

Do you confirm that clearing the payload, moving from parserEvents to writeEvent, can optimize memory usage?

Yes, the test steps are as follows

  1. run ecapture without --tsize
    2.access big file (1.7G+), watch ecapture memory cost, find the peak of memory consumption
  2. run ecapture with --tsize 10240000 (about 10M), repeat step 2
#./bin/ecapture tls
...
#ps -p $(pidof ecapture)  -o rss,%mem
  RSS %MEM
1100172 13.5


#./bin/ecapture tls -t 10240000
# ps -p $(pidof ecapture)  -o rss,%mem
  RSS %MEM
252116  3.1

cfc4n
cfc4n approved these changes May 13, 2025
View reviewed changes
Copy link
Member

@cfc4n cfc4n left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks.

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label May 13, 2025
@cfc4n cfc4n merged commit 91240a9 into gojue:master May 13, 2025
5 checks passed
@chilli13 chilli13 deleted the opt/truncate branch May 30, 2025 01:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cfc4n cfc4n cfc4n approved these changes

+1 more reviewer

@yuweizzz yuweizzz yuweizzz left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
enhancement New feature or request lgtm This PR has been approved by a maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@chilli13 @cfc4n @yuweizzz