Merge pull request #662 from evo-lua/iconv-interface-rework

Rework the interface for the iconv.convert FFI bindings

Author: rdw-software

Benchmarks/iconv-charset-conversion.lua

@@ -1,11 +1,14 @@
 local console = require("console")
 local iconv = require("iconv")
 local ffi = require("ffi")
+local jit = require("jit")
 
 local SAMPLE_SIZE = 500000
 
-local function iconv_lowlevel()
-	local input = "\192\175\192\250\192\206\197\205\198\228\192\204\189\186"
+local UTF_MAX_BYTES_PER_CODEPOINT = 4
+local CP949_INPUT_STRING = "\192\175\192\250\192\206\197\205\198\228\192\204\189\186"
+local UTF8_OUTPUT_STRING = "유저인터페이스"
+local function iconv_lowlevel(input)
 	local descriptor = iconv.bindings.iconv_open("UTF-8", "CP949")
 
 	local inputSize = ffi.new("size_t[1]", #input)
@@ -22,53 +25,85 @@ local function iconv_lowlevel()
 	local converted = ffi.string(outputBuffer, numConversionsPerformed)
 
 	iconv.bindings.iconv_close(descriptor)
-	return converted, result
+	return result, converted
 end
 
-local function iconv_lua()
-	local input = "\192\175\192\250\192\206\197\205\198\228\192\204\189\186"
+local function iconv_lua(input)
 	local output, message = iconv.convert(input, "CP949", "UTF-8")
 	return output, message
 end
 
-local function iconv_cpp()
-	local inputBuffer = buffer.new()
-	local outputBuffer = buffer.new(1024)
-	local ptr, len = outputBuffer:reserve(1024)
-	local result = iconv.bindings.iconv_convert(inputBuffer, #inputBuffer, "CP949", "UTF-8", ptr, len)
-	return result
+-- This should more or less match what iconv.convert is doing, minus validation/error handling
+local readBuffer = buffer.new()
+local writeBuffer = buffer.new()
+local request = ffi.new("iconv_request_t")
+local function iconv_cpp(input)
+	readBuffer:put(input)
+	local readCursor = readBuffer:ref()
+
+	writeBuffer:reset()
+	local writeCursor, writeBufferSize = writeBuffer:reserve(#input * UTF_MAX_BYTES_PER_CODEPOINT)
+
+	request.input.charset = "CP949"
+	request.input.buffer = readCursor
+	request.input.length = #input
+	request.input.remaining = #input
+
+	request.output.charset = "UTF-8"
+	request.output.buffer = writeCursor
+	request.output.length = writeBufferSize
+	request.output.remaining = writeBufferSize
+
+	local result = iconv.bindings.iconv_convert(request)
+	local numBytesWritten = tonumber(request.output.length - request.output.remaining)
+	writeBuffer:commit(numBytesWritten)
+
+	return result, tostring(writeBuffer)
 end
 
 math.randomseed(os.clock())
 local availableBenchmarks = {
 	function()
-		local label = "[FFI] Low-level API (tedious and slow, but the most flexible)"
+		local label = "[FFI] Multi-step conversions using the libiconv API directly (manual descriptor management)"
 		console.startTimer(label)
 		for i = 1, SAMPLE_SIZE, 1 do
-			iconv_lowlevel()
+			local result, output = iconv_lowlevel(CP949_INPUT_STRING)
+			assert(result == ffi.C.ICONV_RESULT_OK, iconv.strerror(result))
+			assert(output == UTF8_OUTPUT_STRING, output)
 		end
 		console.stopTimer(label)
 	end,
 	function()
-		local label = "[FFI] One-shot C++ conversion (fast but less flexible)"
+		local label = "[FFI] Immediate conversion using iconv.bindings.iconv_convert (manual buffer management)"
 		console.startTimer(label)
 		for i = 1, SAMPLE_SIZE, 1 do
-			iconv_cpp()
+			local result, output = iconv_cpp(CP949_INPUT_STRING)
+			assert(result == ffi.C.ICONV_RESULT_OK, iconv.strerror(result))
+			assert(output == UTF8_OUTPUT_STRING, output)
 		end
 		console.stopTimer(label)
 	end,
 	function()
-		local label = "[FFI] Lua-friendly wrapper (safer, but slower)"
+		local label = "[FFI] Immediate conversion using iconv.convert (idiomatic Lua wrapper, with preallocation)"
 		console.startTimer(label)
 		for i = 1, SAMPLE_SIZE, 1 do
-			iconv_lua()
+			local output, message = iconv_lua(CP949_INPUT_STRING)
+			assert(output == UTF8_OUTPUT_STRING, message)
 		end
 		console.stopTimer(label)
 	end,
 }
 
 table.shuffle(availableBenchmarks)
+jit.off()
+print("Running benchmarks with JIT=OFF")
+for _, benchmark in ipairs(availableBenchmarks) do
+	benchmark()
+end
 
+table.shuffle(availableBenchmarks)
+print("Running benchmarks with JIT=ON")
+jit.on()
 for _, benchmark in ipairs(availableBenchmarks) do
 	benchmark()
 end

Runtime/Bindings/FFI/iconv/iconv.lua

@@ -3,87 +3,106 @@ local validation = require("validation")
 
 local validateString = validation.validateString
 
-local ffi_string = ffi.string
 local tonumber = tonumber
 local tostring = tostring
 
-local iconv = {
-	errorMessages = {
-		INVALID_CONVERSION_HANDLE = "Cannot close an invalid iconv_t descriptor",
-	},
-}
+local UTF_MAX_BYTES_PER_CODEPOINT = 4
+
+local iconv = {}
 
 iconv.cdefs = [[
 typedef void* iconv_t;
-typedef struct iconv_result_t {
-	uint8_t status_code;
-	size_t num_bytes_written;
-	const char* message;
+typedef enum iconv_result_t {
+	ICONV_RESULT_OK,
+	ICONV_INVALID_REQUEST,
+	ICONV_INVALID_DESCRIPTOR,
+	ICONV_INVALID_INPUT,
+	ICONV_INVALID_OUTPUT,
+	ICONV_CONVERSION_FAILED,
+	ICONV_INCOMPLETE_INPUT,
+	ICONV_WRITEBUFFER_FULL,
+	ICONV_RESULT_LAST,
 } iconv_result_t;
 
+typedef char* iconv_cursor_t;
+typedef const char* iconv_encoding_t; // Aliased for now, replace with enum later
+
+typedef struct iconv_memory_t {
+	iconv_encoding_t charset;
+	iconv_cursor_t buffer;
+	size_t length;
+	size_t remaining;
+} iconv_memory_t;
+
+typedef struct iconv_request_t {
+	iconv_memory_t input;
+	iconv_memory_t output;
+	iconv_t handle;
+} iconv_request_t;
+
 struct static_iconv_exports_table {
-	iconv_result_t (*iconv_convert)(char* input, size_t input_size, const char* input_encoding, const char* output_encoding, char* output, size_t output_size);
+
+	// Exports from iconv.h
 	iconv_t (*iconv_open)(const char* input_encoding, const char* output_encoding);
 	int (*iconv_close)(iconv_t conversion_descriptor);
 	size_t (*iconv)(iconv_t conversion_descriptor, char** input, size_t* input_size, char** output, size_t* output_size);
 
-	// Shared constants
-	size_t CHARSET_CONVERSION_FAILED;
+	// Charset conversion API
+	iconv_result_t (*iconv_convert)(iconv_request_t* conversion_details);
+	iconv_result_t (*iconv_try_close)(iconv_request_t* request);
+
+	// Utility methods
+	const char* (*iconv_strerror)(iconv_result_t status);
+	bool (*iconv_check_result)(iconv_t handle);
 };
 
 ]]
 
--- Should probably move this elsewhere?
-local function ffi_strerror(errno)
-	return ffi.string(ffi.C.strerror(errno))
-end
-
 function iconv.initialize()
-	ffi.cdef([[
-		// Should probably move this elsewhere?
-		char *strerror(int errnum);
-	]])
-
 	ffi.cdef(iconv.cdefs)
 end
 
-local UTF_BYTES_PER_CODEPOINT = 4
-
+local request, readBuffer, writeBuffer
 function iconv.convert(input, inputEncoding, outputEncoding)
 	validateString(input, "input")
 	validateString(inputEncoding, "inputEncoding")
 	validateString(outputEncoding, "outputEncoding")
 
-	if #input == 0 then
-		-- Prevents LuaJIT from trying to collect a NULL buffer (= crash)
-		return nil, ffi_strerror(22) -- EINVAL
-	end
-
-	local inputBuffer = ffi.new("char[?]", #input + 1, input) -- Wasteful, but iconv modifies the input buffer
-	local maxOutputBufferSize = #input * UTF_BYTES_PER_CODEPOINT -- Worst case scenario (also wasteful)
-	local outputBuffer = buffer.new(maxOutputBufferSize)
-	local ptr, len = outputBuffer:reserve(maxOutputBufferSize)
-
-	local result = iconv.bindings.iconv_convert(inputBuffer, #input, inputEncoding, outputEncoding, ptr, len)
-
-	local numBytesWritten = tonumber(result.num_bytes_written)
-	outputBuffer:commit(numBytesWritten)
-
-	if tonumber(result.status_code) ~= 0 then
-		local errorMessage = ffi_string(result.message)
-		return nil, errorMessage
+	-- Preallocate resources only when needed; it's somewhat costly otherwise
+	request = request or ffi.new("iconv_request_t")
+	readBuffer = readBuffer or buffer.new(256)
+	writeBuffer = writeBuffer or buffer.new(256)
+
+	readBuffer:put(input)
+	local readCursor = readBuffer:ref()
+	request.input.charset = "CP949"
+	request.input.buffer = readCursor
+	request.input.length = #input
+	request.input.remaining = #input
+
+	-- If the input is empty, reserving a zero-length write buffer may lead to misleading errors
+	local maxRequiredWriteBufferSize = math.max(1, #input * UTF_MAX_BYTES_PER_CODEPOINT)
+
+	writeBuffer:reset()
+	local writeCursor, writeBufferCapacity = writeBuffer:reserve(maxRequiredWriteBufferSize)
+	request.output.charset = "UTF-8"
+	request.output.buffer = writeCursor
+	request.output.length = writeBufferCapacity
+	request.output.remaining = writeBufferCapacity
+
+	local result = iconv.bindings.iconv_convert(request)
+	local numBytesWritten = tonumber(request.output.length - request.output.remaining)
+	writeBuffer:commit(numBytesWritten)
+
+	if result ~= ffi.C.ICONV_RESULT_OK then
+		return nil, iconv.strerror(result)
 	end
 
-	return tostring(outputBuffer), ffi_strerror(0)
+	return tostring(writeBuffer), iconv.strerror(result)
 end
 
-function iconv.try_close(descriptor)
-	if ffi.cast("size_t", descriptor) ~= iconv.bindings.CHARSET_CONVERSION_FAILED then
-		-- Guard this because MINGW64's iconv can't handle closing invalid descriptors
-		return iconv.bindings.iconv_close(descriptor)
-	end
-
-	return nil, iconv.errorMessages.INVALID_CONVERSION_HANDLE
+function iconv.strerror(result)
+	return ffi.string(iconv.bindings.iconv_strerror(result))
 end
 
 return iconv

Runtime/Bindings/FFI/iconv/iconv_exports.h

@@ -1,15 +1,43 @@
-typedef struct iconv_result_t {
-	uint8_t status_code;
-	size_t num_bytes_written;
-	const char* message;
+typedef enum iconv_result_t {
+	ICONV_RESULT_OK,
+	ICONV_INVALID_REQUEST,
+	ICONV_INVALID_DESCRIPTOR,
+	ICONV_INVALID_INPUT,
+	ICONV_INVALID_OUTPUT,
+	ICONV_CONVERSION_FAILED,
+	ICONV_INCOMPLETE_INPUT,
+	ICONV_WRITEBUFFER_FULL,
+	ICONV_RESULT_LAST,
 } iconv_result_t;
 
+typedef char* iconv_cursor_t;
+typedef const char* iconv_encoding_t; // Aliased for now, replace with enum later
+
+typedef struct iconv_memory_t {
+	iconv_encoding_t charset;
+	iconv_cursor_t buffer;
+	size_t length;
+	size_t remaining;
+} iconv_memory_t;
+
+typedef struct iconv_request_t {
+	iconv_memory_t input;
+	iconv_memory_t output;
+	iconv_t handle;
+} iconv_request_t;
+
 struct static_iconv_exports_table {
-	iconv_result_t (*iconv_convert)(char* input, size_t input_size, const char* input_encoding, const char* output_encoding, char* output, size_t output_size);
+
+	// Exports from iconv.h
 	iconv_t (*iconv_open)(const char* input_encoding, const char* output_encoding);
 	int (*iconv_close)(iconv_t conversion_descriptor);
 	size_t (*iconv)(iconv_t conversion_descriptor, char** input, size_t* input_size, char** output, size_t* output_size);
 
-	// Shared constants
-	size_t CHARSET_CONVERSION_FAILED;
+	// Charset conversion API
+	iconv_result_t (*iconv_convert)(iconv_request_t* conversion_details);
+	iconv_result_t (*iconv_try_close)(iconv_request_t* request);
+
+	// Utility methods
+	const char* (*iconv_strerror)(iconv_result_t status);
+	bool (*iconv_check_result)(iconv_t handle);
 };