Skip to content

Fix mini_std fwrite/fread performance issue#275

Open
kwn wants to merge 3 commits into
AmigaPorts:mainfrom
kwn:fix-mini-std
Open

Fix mini_std fwrite/fread performance issue#275
kwn wants to merge 3 commits into
AmigaPorts:mainfrom
kwn:fix-mini-std

Conversation

@kwn
Copy link
Copy Markdown
Contributor

@kwn kwn commented May 18, 2026

fread and fwrite in mini_std/stdio_file.c looped Count times calling Read(Size) / Write(Size). This made disk_file.c's direct read path (fread(buf, ulSize, 1, f)) issue one Read(1) per byte killing load times.

Fixed both to do a single Read(Size * Count) / Write(Size * Count) call, matching standard C return semantics (bytesRead / Size).

Also swapped disk_file.c line 108 to fread(pDestBytes, 1, ulSize, ...) so the return value correctly reflects bytes read.

Tested with Bartman toolchain. Please verify with bebbo to make sure nothing breaks there.

Comment thread src/ace/utils/disk_file.c
Copy link
Copy Markdown
Collaborator

@Vairn Vairn May 19, 2026

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

Comment thread src/mini_std/stdio_file.c
Copy link
Copy Markdown
Collaborator

@Vairn Vairn May 19, 2026

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

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.

Copy link
Copy Markdown
Collaborator

@Vairn Vairn left a comment

Choose a reason for hiding this comment

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

This is closer to a the std fread/fwrite, but not sure if we need 100% compatible.

Copy link
Copy Markdown
Member

@tehKaiN tehKaiN left a comment

Choose a reason for hiding this comment

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

Great job!

Comment thread src/ace/utils/disk_file.c
fileAccessEnable();
}
ULONG ulReadPartSize = fread(pDestBytes, ulSize, 1, pDiskFileData->pFileHandle);
ULONG ulReadPartSize = fread(pDestBytes, 1, ulSize, pDiskFileData->pFileHandle);
Copy link
Copy Markdown
Member

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.

Comment thread src/mini_std/stdio_file.c
if(Size == 0 || Count == 0) return 0;
lBytesRead = Read((BPTR)pStream, pBuffer, Size * Count);
if(lBytesRead <= 0) return 0;
return (size_t)lBytesRead / Size;
Copy link
Copy Markdown
Member

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

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

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.

Copy link
Copy Markdown
Member

@tehKaiN tehKaiN left a comment

Choose a reason for hiding this comment

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

One thing that I didn't catch earlier. Please change that and we should be good to go.

Other than that, I've tested it on Bebbo compiler and it works nicely.

Comment thread src/mini_std/stdio_file.c
pByteBuffer += Size;
}
return BytesRead;
if(Size == 0 || Count == 0) return 0;
Copy link
Copy Markdown
Member

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

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