Cynerd commented on code in PR #7198: URL: https://github.com/apache/incubator-nuttx/pull/7198#discussion_r1001587648
########## libs/libc/obstack/lib_obstack_finish.c: ########## @@ -0,0 +1,57 @@ +/**************************************************************************** + * libs/libc/obstack/lib_obstack_finish.c + * + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. The + * ASF licenses this file to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the + * License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + * + ****************************************************************************/ + +/**************************************************************************** + * Included Files + ****************************************************************************/ + +#include <obstack.h> +#include <nuttx/lib/lib.h> +#include <assert.h> + +/**************************************************************************** + * Public Functions + ****************************************************************************/ + +FAR void *obstack_finish(FAR struct obstack *h) +{ + size_t chsize; + void *chbase; + + chbase = (FAR char *)h->chunk + sizeof(struct _obstack_chunk); + if (chbase == h->object_base) + { + chsize = h->next_free - (FAR char *)h->chunk; + h->chunk = lib_realloc(h->chunk, chsize); + h->chunk->limit = (FAR void *)h->chunk + chsize; + h->object_base = h->chunk->limit; + h->next_free = h->chunk->limit; + return (FAR void *)h->chunk + sizeof(struct _obstack_chunk); + } + + return obstack_finish_norealloc(h); +} + +FAR void *obstack_finish_norealloc(FAR struct obstack *h) Review Comment: That is the question.. Honestly, the point of even having this "non-standard" behaviour is to reduce the number of unused bytes when growing objects. The default `BUFSIZ` in the case of NuttX is `64`, which is not much but also can be too much-unused space. The overhead of creating the new chunk in terms of memory is two words (thus `8` bytes on 32bit). The memory overhead of growing small and big objects in tandem can be "pretty big". Let's consider the worst case of growing one byte, finishing, growing 57 (`64 - 8 + 1`) bytes and finishing that. That results in three allocated blocks and thus 192 bytes with 110 unused bytes (if I calculated correctly). Compare that with reallocating finish which would use 74 bytes (`8+1+8+57`). On the other hand, the worst the case scenario for the reallocating finish is, of course, allocating a lot of single bytes. You can fit 56 of such bytes into the single block while reallocating would add 8 bytes overhead to every such allocation. Thus we would use `504` bytes i nstead of `64`. The question is how often the code is going to be using obstack for allocating single bytes (I think that it is unlikely, and the first case is more likely). At the same time, the worst-case scenario for the reallocating finish looks simply terrible. Another way to look at it, I think, is the question if we want to keep the original behaviour or change it. I see reasons for changing it here, especially because I appreciate the memory reduction if you allocate data with varied sizes. My last remark is that the best solution is a reallocation that won't move the base (fails if that is impossible). I could not get this with nuttx's allocator, but that might be just that I missed that in the code somewhere. The advantage would be that we could try to increase chunk size even if there is already a finished object. If that fails, we could use it to reduce allocation size and thus free those bytes we cannot use. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
