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

Amend File API design document to address file.Close constraints #3328

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

oleiade
Copy link
Member

@oleiade oleiade commented Sep 8, 2023

What?

This Pull Request updates the File API design document, focusing on the limitations of the file.Close operation, especially when files are opened in the init context.

Why?

The current design treats the file.Close operation as a non-operation, as detailed in #3314. This choice was by design, as we anticipated changing this once k6 can open files during the VU stage.

We've explored an alternative where k6 tracks how many times a file has been opened. When this count reaches zero, k6 would free up the memory holding the file content. More details can be found in the proof-of-concept for this approach.

A challenge arose: opening files in the init context means the count might never reach zero. This is because the file keeps a reference until the test concludes.

After discussions with @codebien, we agreed that due to k6's lifecycle stages and the init context's frequent calls, the best approach would be to:

  • Adopt the reference count approach.
  • Keeping data in memory as long as the init context code hasn't closed the file to a point where zero references are left.
  • Close the file and free memory only at the test's end, possibly using the newly added event system.

When #3020 is in place, our plan is to adjust this so that files opened exclusively in the VU stage can have their memory released earlier, given they are consistently closed in that stage.

Refs

#3149
#3101
#3314
#3020

@oleiade oleiade changed the base branch from master to experimental/fs September 8, 2023 14:26
@oleiade oleiade force-pushed the design-docs/file-api-close branch 2 times, most recently from c73eeed to afed1ec Compare September 8, 2023 14:28
@oleiade oleiade changed the base branch from experimental/fs to master September 8, 2023 14:31
@codecov-commenter
Copy link

codecov-commenter commented Sep 8, 2023

Codecov Report

Attention: 108 lines in your changes are missing coverage. Please review.

Comparison is base (c944ebf) 73.16% compared to head (f1ebad0) 73.33%.
Report is 65 commits behind head on master.

❗ Current head f1ebad0 differs from pull request most recent head 4528f56. Consider uploading reports for the commit 4528f56 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3328      +/-   ##
==========================================
+ Coverage   73.16%   73.33%   +0.16%     
==========================================
  Files         258      258              
  Lines       19912    19645     -267     
==========================================
- Hits        14569    14407     -162     
+ Misses       4418     4353      -65     
+ Partials      925      885      -40     
Flag Coverage Δ
ubuntu 73.27% <50.68%> (+0.18%) ⬆️
windows 73.18% <50.68%> (+0.16%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
cmd/root.go 94.26% <100.00%> (ø)
js/bundle.go 85.90% <100.00%> (ø)
js/console.go 94.28% <100.00%> (+0.16%) ⬆️
js/modules/k6/http/request.go 84.27% <100.00%> (+0.09%) ⬆️
js/summary.go 89.87% <100.00%> (ø)
lib/consts/consts.go 51.61% <ø> (ø)
lib/netext/grpcext/conn.go 88.88% <100.00%> (+3.17%) ⬆️
lib/netext/httpext/batch.go 100.00% <ø> (ø)
output/cloud/expv2/hdr.go 100.00% <100.00%> (ø)
output/csv/config.go 74.50% <ø> (-2.69%) ⬇️
... and 9 more

... and 4 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@oleiade oleiade mentioned this pull request Oct 4, 2023
@oleiade oleiade self-assigned this Oct 4, 2023
Copy link
Collaborator

@codebien codebien left a comment

Choose a reason for hiding this comment

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

@oleiade We should move this change to the possible future improvements section and update it by integrating #3149 (comment).

@oleiade
Copy link
Member Author

oleiade commented Oct 26, 2023

Agreed 👍🏻

What did you refer to as the possible future improvements section?

@codebien
Copy link
Collaborator

@oleiade
Copy link
Member Author

oleiade commented Oct 26, 2023

@codebien I see, you meant the one in the file directly 🙇🏻 Fair enough, I'll amend the document 👍🏻

@oleiade
Copy link
Member Author

oleiade commented Oct 26, 2023

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants