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

Reply via email to