Closed Bug 787291 Opened 7 years ago Closed 7 years ago

add pod_malloc<T> et al

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: luke, Assigned: luke)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
This patch lets you change:
  T *t = (T *)cx->malloc_(sizeof(T));
  T *ts = (T *)cx->malloc_(numElems * sizeof(T));
to:
  T *t = cx->pod_alloc<T>();
  T *ts = cx->pod_alloc<T>(numElems);

It also adds a similar pod_calloc and global non-accounting js_pod_malloc/js_pod_calloc.

In addition to avoiding all the noisy syntax and increasing type safety, the numElems-taking overloads guard against overflow when multiplying by sizeof(T) (which, using template magic, is a single bit test).

Going through all calls to malloc_/calloc_/js_malloc/js_calloc, the vast majority can be rewritten to use this new pod_* variants.
Attachment #657105 - Flags: review?(wmccloskey)
Attached patch patchSplinter Review
Whitespace fix.
Attachment #657105 - Attachment is obsolete: true
Attachment #657105 - Flags: review?(wmccloskey)
Attachment #657108 - Flags: review?(wmccloskey)
Comment on attachment 657108 [details] [diff] [review]
patch

Review of attachment 657108 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/BytecodeEmitter.cpp
@@ +2369,5 @@
>                      intmap_bitlen = INTMAP_LENGTH << JS_BITS_PER_WORD_LOG2;
>                  } else {
>                      /* Just grab 8K for the worst-case bitmap. */
>                      intmap_bitlen = JS_BIT(16);
> +                    intmap = cx->pod_malloc<jsbitmap>((JS_BIT(16) >> JS_BITS_PER_WORD_LOG2));

Eliminate double parens.

::: js/src/jscntxt.h
@@ +933,5 @@
>      }
>  
> +    template <class T>
> +    T *pod_malloc(JSContext *cx = NULL) {
> +        return (T *)malloc_(sizeof(T));

Don't you want to pass in cx here and below?

@@ +1435,5 @@
>      inline void* realloc_(void* p, size_t oldBytes, size_t newBytes) {
>          return runtime->realloc_(p, oldBytes, newBytes, this);
>      }
>  
> +    template <class T> T *pod_malloc(JSContext *cx = NULL) {

I don't think this needs a cx argument. Or the one below.
Attachment #657108 - Flags: review?(wmccloskey) → review+
Comment on attachment 657108 [details] [diff] [review]
patch

Review of attachment 657108 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/public/Utility.h
@@ +21,5 @@
>  
>  #include "jstypes.h"
>  
>  #ifdef __cplusplus
> +# include "TemplateLib.h"

"js/TemplateLib.h", I think

::: js/src/frontend/BytecodeEmitter.cpp
@@ -2516,5 @@
>               * Use malloc to avoid arena bloat for programs with many switches.
>               * ScopedFreePtr takes care of freeing it on exit.
>               */
>              if (tableLength != 0) {
> -                tableSize = (size_t)tableLength * sizeof *table;

Please eliminate the declaration too

::: js/src/jsdbgapi.cpp
@@ +360,4 @@
>      if (!lines)
>          return JS_FALSE;
>  
> +    pcs = cx->pod_malloc<jsbytecode*>(len);

Bonus points for moving the declarations of those variables down a bit.
(In reply to Bill McCloskey (:billm) from comment #2)
> Don't you want to pass in cx here and below?
...
> I don't think this needs a cx argument. Or the one below.

Sheesh, yes, good catches.  Multitasking is bad for patches.
https://hg.mozilla.org/mozilla-central/rev/949ade2a2ea1
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.