Skip to content

Fix mini_std fwrite/fread performance issue #275

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

Open
kwn wants to merge 3 commits into
base: main
Choose a base branch
from
+13 −16
Open

Fix mini_std fwrite/fread performance issue #275

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c709a34
Fix mini_std fwrite/fread
kwn May 18, 2026
6ac79cb
Post review udpate
kwn May 22, 2026
e0ca4ec
Add missing header
kwn May 22, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/ace/utils/disk_file.c
View file
Open in desktop
Copy link
Copy Markdown
Collaborator

@Vairn Vairn May 19, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

yeah, the first one would return 1, while the second one should return the amount read.

Since this is a buffered read system v1 is broken, while v2 would behave as required.

But it looks like the fread/fwrite don't work the standard way. anthow, the new commit is closer to the std tho

Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ DISKFILE_PRIVATE ULONG diskFileRead(void *pData, void *pDest, ULONG ulSize) {
if(!pDiskFileData->isUninterrupted) {
fileAccessEnable();
}
ULONG ulReadPartSize = fread(pDestBytes, ulSize, 1, pDiskFileData->pFileHandle);
ULONG ulReadPartSize = fread(pDestBytes, 1, ulSize, pDiskFileData->pFileHandle);
Copy link
Copy Markdown
Member

@tehKaiN tehKaiN May 19, 2026

Choose a reason for hiding this comment

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

Good catch! I've changed it only in one place and skipped this one.

pDestBytes += ulReadPartSize;
ulReadCount += ulReadPartSize;
ulSize -= ulReadPartSize;
Expand Down
27 changes: 12 additions & 15 deletions src/mini_std/stdio_file.c
View file
Open in desktop
Copy link
Copy Markdown
Collaborator

@Vairn Vairn May 19, 2026 •
edited by tehKaiN
Loading

Choose a reason for hiding this comment

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

hmm, if you want to to match the c std, I think it would be closer to this.

size_t fread(void *restrict pBuffer, size_t Size, size_t Count, FILE *restrict pStream) {
    if(Size == 0 || Count == 0) return 0;

    // Check for overflow before multiplying
    if(Count > SIZE_MAX / Size) return 0;

    LONG lBytesRead = Read((BPTR)pStream, pBuffer, Size * Count);

    if(lBytesRead < 0) {
        // Read() returns -1 on error (IoErr() has details)
        // fread should set stream error indicator — skipped if no FILE struct
        return 0;
    }
    if(lBytesRead == 0) {
        // EOF — set EOF indicator if FILE struct supports it
        return 0;
    }

    // Return complete items only — tail partial item is "not read"
    // This matches C standard: partial final item is discarded from count
    return (size_t)lBytesRead / Size;
}

size_t fwrite(const void *restrict pBuffer, size_t Size, size_t Count, FILE *restrict pStream) {
    if(Size == 0 || Count == 0) return 0;

    if(Count > SIZE_MAX / Size) return 0;

    LONG lBytesWritten = Write((BPTR)pStream, (void *)pBuffer, Size * Count);

    if(lBytesWritten < 0) {
        // Write() returns -1 on error
        return 0;
    }

    return (size_t)lBytesWritten / Size;
}

Copy link
Copy Markdown
Contributor Author

@kwn kwn May 22, 2026

Choose a reason for hiding this comment

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

Hey @Vairn . Thanks for the feedback and pointing out shortcomings.

I updated the code. I persisted comments in my brain, but removed them from the code (except one) to make it aligned with the minimalistic style of ACE.

Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#include <stdio.h>
#include <ace/types.h>
#include <proto/dos.h>
#include <stdint.h>

// Some things are implemented from scratch, some are based on:
// https://github.com/deplinenoise/amiga-sdk/blob/master/netinclude/stdio.h
Expand Down Expand Up @@ -36,25 +37,21 @@ FILE *fopen(const char *restrict szFileName, const char *restrict szMode) {

size_t fread(void *restrict pBuffer, size_t Size, size_t Count, FILE *restrict pStream) {
// http://amigadev.elowar.com/read/ADCD_2.1/Includes_and_Autodocs_3._guide/node01A0.html
unsigned char *pByteBuffer = (unsigned char *)pBuffer;
size_t BytesRead = 0;
while(Count--) {
// FIXME: handle 0/negative vals
BytesRead += Read((BPTR)pStream, pByteBuffer, Size);
pByteBuffer += Size;
}
return BytesRead;
if(Size == 0 || Count == 0) return 0;
Copy link
Copy Markdown
Member

@tehKaiN tehKaiN May 22, 2026

Choose a reason for hiding this comment

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

please use braces for instructions inside if at all times, with each of them being in new line

if(Count > SIZE_MAX / Size) return 0;
// Read() returns -1 on error (IoErr() has details)
LONG lBytesRead = Read((BPTR)pStream, pBuffer, Size * Count);
if(lBytesRead <= 0) return 0;
return (size_t)lBytesRead / Size;
Copy link
Copy Markdown
Member

@tehKaiN tehKaiN May 19, 2026

Choose a reason for hiding this comment

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

That's definitely better! Could be improved with @Vairn suggestions though.

Copy link
Copy Markdown
Contributor Author

@kwn kwn May 19, 2026

Choose a reason for hiding this comment

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

Ok, let me give it a go

Copy link
Copy Markdown
Contributor Author

@kwn kwn May 22, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Hey @tehKaiN. I updated the code according to @Vairn's suggestions.

}

size_t fwrite(const void *restrict pBuffer, size_t Size, size_t Count, FILE *restrict pStream) {
// http://amigadev.elowar.com/read/ADCD_2.1/Includes_and_Autodocs_3._guide/node01D1.html
unsigned char *pByteBuffer = (unsigned char *)pBuffer;
size_t BytesWritten = 0;
while(Count--) {
BytesWritten += Write((BPTR)pStream, pByteBuffer, Size);
pByteBuffer += Size;
}
return BytesWritten;
if(Size == 0 || Count == 0) return 0;
if(Count > SIZE_MAX / Size) return 0;
LONG lBytesWritten = Write((BPTR)pStream, (void *)pBuffer, Size * Count);
if(lBytesWritten < 0) return 0;
return (size_t)lBytesWritten / Size;
}

int fclose(FILE *pStream) {
Expand Down