Skip to content

Replace legacy Base64 implementation with libbase64 - #116

Draft
Belonit wants to merge 4 commits into
OpenTS-Developers:mainfrom
Belonit:replace-base64-codec
Draft

Replace legacy Base64 implementation with libbase64#116
Belonit wants to merge 4 commits into
OpenTS-Developers:mainfrom
Belonit:replace-base64-codec

Conversation

@Belonit

@Belonit Belonit commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

The custom Base64 codec is legacy code used only for INI UUBlock serialization.
This replaces it with SIMD-accelerated libbase64 while keeping the existing UUBlock format compatible.

The work is split into four commits:

  1. Package third-party licenses with releases is the same change as Package third-party licenses with releases #115.
    I would prefer Package third-party licenses with releases #115 to be merged first, so this commit can be dropped when the branch is rebased.
  2. Add INI UUBlock compatibility tests records the behavior of the old implementation before replacing it.
  3. Replace legacy Base64 implementation with libbase64 replaces the codec and removes its unused stream adapters.
  4. Update INI UUBlock compatibility tests makes the same tests build against the new implementation.

The Base64 test changes are separate because I am not sure whether they should be included upstream.

@Belonit Belonit changed the title Replace base64 codec Replace legacy Base64 implementation with libbase64 Sep 2, 2026
@Belonit
Belonit marked this pull request as draft September 2, 2026 22:07
Comment thread code/ini.cpp
int outcount = b64pipe.Put(buffer, length);
total += outcount;
int filtered_count = 0;
for (int offset = 0; offset < length; offset++) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What is all this, why is it needed?
The input should already be base64, I dont think the original implementation deviated?

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.

The original decoder was different here: Base64_Decode explicitly ignored every character outside the Base64 alphabet and stopped at the first "=" character. Get_UUBlock reached it via Base64Pipe, so the existing permissive decoding behavior for old INI data is preserved.

I agree that this is more code than is required for the normal path.
filtering exists only for compatibility with the old decoder.

If we do not consider support for damaged/non-standard UUBlocks important, then the code in this place can be greatly simplified

Comment thread code/ini.cpp
* *
* HISTORY: *
* 07/02/1996 JLB : Created. *
* 09/02/2026 Belonit : Switched Base64 decoding to libbase64. *

@tomsons26 tomsons26 Sep 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Generally we don't modify official comments, if we have to be modified they become our comment format, in History case not sure what to do yet..

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.

I don't quite understand how to deal with comments specifically in this case

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.

I've been amending WW comments when the change isn't fundamental. don't update the history notes tho.

@tomsons26

Copy link
Copy Markdown
Contributor

Is there a benchmark driven reason for this?
Original code was simple and compact, unless the perf gain is massive from this library i don't see a point in doing changes for change sake

@ZivDero

ZivDero commented Sep 3, 2026

Copy link
Copy Markdown
Member

Can we keep the pipe/straw classes? They're idiomatic here. Replace the actual base64 impl instead?

@Belonit

Belonit commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

Is there a benchmark driven reason for this?
Original code was simple and compact, unless the perf gain is massive from this library i don't see a point in doing changes for change sake

UUBlock encoding and decoding are not per-frame hot paths, so I do not think a benchmark is necessary. libbase64 should still be significantly faster for large blocks, and I can measure that if useful.

The primary motivation was maintenance: the bespoke codec plus Base64Pipe and Base64Straw were only used for UUBlock handling. libbase64 replaces that isolated implementation with a tested library while preserving the stored format

@Belonit

Belonit commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

Can we keep the pipe/straw classes? They're idiomatic here. Replace the actual base64 impl instead?

We can, but these are not general-purpose adapters in the current tree: Base64Pipe and Base64Straw are only used by the two UUBlock functions.

They also buffer a single Base64 quantum (PBuffer[3] / CBuffer[4]). Replacing only Base64_Encode and Base64_Decode would call libbase64 on 3 or 4 bytes at a time, which removes any meaningful SIMD benefit. To use libbase64 effectively, the adapters would need larger batching buffers and become a larger rewrite than the direct UUBlock implementation.

Keeping them is reasonable if preserving the local Pipe/Straw shape is more important than removing the unused abstraction. Otherwise, I think calling the library directly is the simpler result.

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