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

Stream output files instead of writing them in-memory first #46

Merged

Conversation

Mikolaj-A-Kowalski
Copy link
Collaborator

This resolves Issue #38. However, I cannot help but to think it is a bit filthy... that being said I am not sure how exactly to make it nicer.

The problem with streaming to the output file directly is that it is convenient to have a 'backtrace' capability, when writing the printers for different output formats. This allows, for example, to remove trailing commas with relative ease (crucial for JSON).

To 'have a cake and eat it' this implements a delayedStream which works a bit like a buffered output, but when the buffer is flushed it retains MIN_SIZE=16 last characters [as opposed to printing everything to the disk].

What I don't like about the changes is the way the file ownership is handled. It is made to be a responsibility of the outputFile class, which opens a file in initand closes it in finalisation (which is also the destructor). However, the printing is implemented by the asciiOutput_inter which owns an instance of delayedStream. The appropriate output unit is set by a subroutine.

The other thing I fear may be confusing is that if one does not supply filename to outputFile no file will be produced (this is required for unit testing).

In short this PR may still require some iterations [in addition to squashing the fixups!]

@Mikolaj-A-Kowalski Mikolaj-A-Kowalski added the enhancement New feature or request label Jul 4, 2023
@@ -23,6 +23,7 @@ module dummyPrinter_class

contains
procedure :: init
procedure :: endFile
procedure :: extension
procedure :: writeToFile
Copy link
Member

Choose a reason for hiding this comment

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

I thought writeToFile was gone in the asciiOutput class?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is... I think this is an oversight that this got left in.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should be removed now.

! It causes buggy warnings to appear in compilation
allocate(self % outputFileName, source = trim(filename) // "." // self % output % extension())

open(newunit = self % outputUnit, &
Copy link
Member

Choose a reason for hiding this comment

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

It looks like 'error' is gone from this list.

Copy link
Member

Choose a reason for hiding this comment

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

same with errorMsg

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

😨 Ups... not great thing to forget

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should be fixed.

! Close file
close(unitNum)
!! NOTE:
!! This subroutine WILL NOT be called if the `end program` statement is
Copy link
Member

Choose a reason for hiding this comment

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

This comment is not clear.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  !!  NOTE:
  !!    Being marked `final`(a deconstructor)  this subroutine will be normally implicitly called by
  !!    by the compiler whenever a variable of `type(outputFile)` goes out of scope (e.g. when program 
  !!    execution reaches `end subroutine`, `end function` or `end block` statement. However, the rule in 
  !!    Fortran is that the decostructors are NOT called at the end of the program `end program`. In that case 
  !!    if one uses the output file as a module variable or defines it inside the program block, the `finalisation` 
  !!    must be called explicitly to ensure or data from the buffer is written to the disk 
  !!     

Does this work better?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Comment has been changed.

Copy link
Member

@valeriaRaffuzzi valeriaRaffuzzi left a comment

Choose a reason for hiding this comment

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

See comments.

@Mikolaj-A-Kowalski
Copy link
Collaborator Author

So I think I have addressed all the issues now...
If it looks ok I will rebase on the current main (and squash all the fixups!) and then it should be ready to be merged.

docs/Output.rst Outdated Show resolved Hide resolved
Copy link
Member

@valeriaRaffuzzi valeriaRaffuzzi 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, I think this all looks great and it's a much better alternative to write the output file than keep it all in memory!

I think there's just a small leftover typo that I spotted in the docs (soon SCONE will be typo free!!!) but then feel free to rebase and merge

Mikolaj-A-Kowalski and others added 3 commits January 26, 2024 13:42
Allows the outputFile to print to disk anytime between initialisation
and finalisation. Thus, will allow to reduce memory usage.
A delay buffer is needed to be able to make small modifications to
already written data (e.g. remove a trailing comma), which makes the
implementation of output formats much easier.
Co-authored-by: valeriaRaffuzzi <108435337+valeriaRaffuzzi@users.noreply.github.com>
@Mikolaj-A-Kowalski Mikolaj-A-Kowalski merged commit 128f3ef into CambridgeNuclear:main Jan 26, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants