"Allocations Not Freed" in strings, or am I doing something incorrectly?

Help
1

I’ve been noticing high memory usage in a CSV scanning program I’m writing. After enabling debug I am now seeing “Allocations Not Freed” errors with string operations within a loop. Since I’m just getting started with Odin I’m not sure if it’s something I’m doing wrong, or if there are small leaks (typically < 200 bytes) within the strings package itself?

The variables I’m putting things into are defined as:

    csv_line_struct :: struct {
        filepath : string,
        filebytes : u64,
        filedate : time.Time,
        filecount : u64
        }
    csv_input_line : csv_line_struct
    work_filepath : string

The “r” below is a csv.iterator_next row, and inside the loop for that I’m doing (possibly awful) things to chop up various CSV fields, such as:

        	work_filepath = strings.trim(strings.split(r[0],"/")[1],"/. ")
 			csv_input_line.filepath = strings.clone(work_filepath)
			csv_input_line.filebytes, _ = strconv.parse_u64_of_base(r[1],10)
				workmonth, _ = strconv.parse_uint(strings.split(r[2],"/")[0])
				workday, _ = strconv.parse_uint(strings.split(r[2],"/")[1])
				workyear, _ = strconv.parse_uint(strings.split(r[2],"/")[2])

Later on I append that csv_input_line to an output array of csv_line_struct. I had to do the strings.clone because I discovered if I didn’t then that string field didn’t survive exiting the iterator loop and became garbage for…reasons?

Every line above has a leak except the “csv_input_line.filebytes” one.

I’m hoping this is just some rookie crap on my part, so forgive me in advance.

FYI - the small CSV test case for this is only about 100,000 lines, however the ultimate final run is against a nearly 300m row CSV file (yikes, I know) so even small leaks add up super-quickly in that scenario, into many many dozens of GB of bloat.

2

Odin has a builtin core:encoding/csv package (unless of course you’re re-implementing it for educational purposes).

strings.split allocates a slice of strings which you need to free using a call to ‘delete’. Also, you’re making a lot of useless allocations by just throwing away the results of strings.split, just do this:

date := strings.split(r[2], "/")
defer delete(data)

workmonth, _ = strconv.parse_uint(date[0])
workday,   _ = strconv.parse_uint(date[1])
workyear,  _ = strconv.parse_uint(date[2])

Likewise, strings.clone also heap allocates memory by default. You should check out the procedure signatures, those that take an allocator parameter do so for a reason. :stuck_out_tongue:

1 Like
3

The most glaring issue is the use of the strings.split proc, for which the documentation says this:

  • Allocates Using Provided Allocator
  • NOTE: Allocation occurs for the array, the splits are all views of the original string.

Meaning that you own the returned slice of strings, and not deleting it will cause a memory leak.

The reason that clone is necessary, is probably because some or all reuse settings on the csv reader are enabled, if I’d have to guess.
That is assuming you are using core:encoding/csv, which has worse documentation on the individual procedures than e.g. strings.

It does seem like you can largely process things while iterating, so I think those reuse settings are actually a good idea.
It’s just that you need to think about which things need to…:

  1. … live longer, and clone those, preferably allocated together in e.g. an arena, depending on lifetime needs.
  2. … can be done during the iteration in which the returned record is valid, and preferably without further allocations on the csv fields (e.g. replacing strings.split with strings.split_iterator or your own logic).
4

I am 100% not re-implementing CSV for any reason, I’m using it as-is.

@margal - Thanks for the clarification on allocations, suggestions on optimizations, and a clear code example. See below for the final (for now) version.

@SaiMoen - The clone seems to still be necessary to have the string portion of the output array actually exist once it exits the main iterator for-loop. I’m not sure if that’s due to anything CSV related or just strings being strings in Odin. I did simplify things (below) since I realized I only need to clone every time I’m creating a new unique record, which is really, really not that often the bigger this data set gets. Good thing too, now it only leaks bytes for those new record inserts.

Background: The big input file is a whopping 298,142,760 rows of filesystem analytic data, and the goal here was to summarize by base directory (first element of each path) the totals for each of those, along with the oldest thing in each one. Across that entire data set there were only 143 top-level base directories.

So, results?

I went from leaking out 5x per row, north of 100gb ( :fire: ) of RAM, to now leaking maybe a few kb worth. Total runtime RAM usage is now < 2mb. It also chews through those 298m rows in 100 seconds flat, so it’s averaging 2.9m rows/second… which doesn’t suck.

Here’s the main per-row loop now:

	for r, i in csv.iterator_next(&r) {
        matched_line = false
        if len(r[0]) > 1 {
        	line_count += 1
        	if line_count % 100000 == 0 {
        		fmt.printfln ("Read %d lines",line_count)
        	}
        	tmp_filepath := strings.split(r[0],"/")
        	defer delete (tmp_filepath)
        	work_filedate := strings.split(r[2],"/")
        	defer delete (work_filedate)
	       	filepath = strings.trim(tmp_filepath[1],"/. ")
			filebytes, _ = strconv.parse_u64_of_base(r[1],10)
				workmonth, _ = strconv.parse_uint(work_filedate[0])
				workday, _ = strconv.parse_uint(work_filedate[1])
				workyear, _ = strconv.parse_uint(work_filedate[2])
			filedate, _ = time.components_to_time(workyear, workmonth, workday, 0, 0, 0, 0)
			filecount = 1
			for &tmp_array_line in csv_output_array {
				if (strings.compare(tmp_array_line.filepath,filepath)) == 0 {
 					matched_line = true
					tmp_array_line.filebytes += filebytes
					tmp_array_line.filecount += filecount
					if time.to_unix_nanoseconds(filedate) < time.to_unix_nanoseconds(tmp_array_line.filedate) {
					   tmp_array_line.filedate = filedate
	                }
				break
				}
				else {
					continue
				}
			} 
			if matched_line == false {
				append_soa(&csv_output_array, csv_line_struct{strings.clone(filepath),filebytes,filedate,filecount})
			}
		}
 	}	```
5

Nice that your problem is solved! If something is called tmp_something and you basically delete it directly after allocating it, you might as well use the context.temp_allocator to allocate it and never delete it explicitly. Just call free_all(context.temp_allocator) after each row.

Edit: Of course, this goes for all allocations that don’t need to persist outside the current row.

2 Likes
6

Sorry for the misunderstanding. I was a little bit tipsy and didn’t figure out that you were talking about the csv package itself. Luckily I was at least somewhat useful. :crazy_face:

GigaGrunch’s suggestion about using temp allocators, where appropriate, is good. You’d do something like tmp_filepath := strings.split(r[0],"/", context.temp_allocator) and then you don’t need to call delete(tmp_filepath) on it and just do free_all(context.temp_allocator) after each row.

Since you’re now leaking only a few kilobytes, I suppose the memory leaks are no longer inside of a loop. By the way, I’m assuming you’re using the tracking memory allocator to find memory leaks?

7

A string in Odin is just a byte slice that prevents setting bytes, and so it consists of a pointer and a length (there is no magic going on). As I mentioned earlier, the clone is necessary because of the reuse fields on the csv reader (though I cannot see how you configured it, so if those are actually false it’d have to be something else).

The string clone will of course allocate, as well as the append_soa, so it could be either or both. If these happen at the top level of the whole program, you could just leak and the OS will clean it up. Otherwise you’d have to delete whatever is leaking.

Although it seems your problem has already been solved, if optimizations are interesting to you, here are some things I can see at a quick glance:

  • If you look at the access patterns of tmp_filepath and work_filedate, you can see that you don’t even need to allocate at all. You could replace those two usages of strings.split with just finding the indices of those "\" (using e.g. strings.index) and passing slices of the csv field directly to the parse procedures. The others also mentioned a good middle ground (w.r.t. convenience), through the use of temp_allocator with strings.split. I also recommended strings.split_iterator earlier, but that one actually modifies the input string so it would be a bad fit.
  • If the strings.clone(filepath) is leaking, and you want to clean it up, you could consider using an arena for that (though do not use temp_allocator if you are also freeing it on each iteration to use it with strings.split).
  • The loop through the entire output array inside of the main loop could be condensed into a map lookup for that filepath instead.
8

if you want to free up the cloned strings then after you are done with the csv_output_array you’ll need to iterate over csv_output_array and delete every filepath in it. Then delete the csv_output_array itself.

9

@margal I am using the memory tracker from OnlyXuul, mainly because it is prettier. The credits say it is based on the ginger bill and Zylinski ones. I will say, tracking leaks this way is much better than either Rust’s “Nope, not gonna compile that, and you’re an idiot btw.” or Python’s “Trust me bro, you’ve got plenty of RAM” approach.

@SaiMoen I do have everything configured to TRUE for CSV, so that explains the clone. The clone is where the leak is, not the append - it’ll happily move to whatever line I put it in, I just left it inline there. At least it is trivial now. I tried an arena before but was missing the fundamentals about cleanup y’all pointed out before, and I may explore that again now that I have it down to a more reasonable level.

I also considered using a map for all this, and the version I wrote of it in Rust used that. I wanted to start out with something more simple here, since this is literally my very first Odin program. I did not use any kind of map for the original (godawful slow) Python version or the Go version. I may go back and explore maps with Odin at some point.

For historical comparison, the Python version of this took hours to run, Go was much faster, Rust was (I believe) a bit faster than Go, and Odin’s right up there close to Rust. Obviously the compiled language times were always going to be a lot faster, but the difference was pretty stark.

Personal background: If you haven’t guessed by now, I’ve been creeping around the Internet Tubes in general (I have a 4-digit Slashdot ID, for instance) and programming in particular for quite some time now , but it isn’t something I’ve done full-time for a living in decades. I’ve always written code of various kinds to do various things along the way, but the big work has been in infrastructure and DBs. I’m doing all this to keep the old skills sharp while solving some real-world problems, with an eye towards future side projects after the main work is done.

I appreciate everyone’s help here!