Repository navigation
Bug/memory bloat fix - #4
Conversation
asldevi
left a comment
There was a problem hiding this comment.
overall it looks good @ShivamYuvraj.
we should add at least two testcases at php/c level, whichever is easier.
- to check that everything is sane with less than DEFAULT_SPAN_LIMIT number of spans
- to check that everything is sane with >= DEFAULT_SPAN_LIMIT number of spans
| * Static method to add tracer | ||
| */ | ||
| public static function setTracer($tracer){ | ||
| PDO::$tracer = $tracer; |
There was a problem hiding this comment.
hv made some change in this logic
| static void (*opencensus_original_zend_execute_ex) (zend_execute_data *execute_data); | ||
| static void (*opencensus_original_zend_execute_internal) (zend_execute_data *execute_data, zval *return_value); | ||
| // global value for max number of spans in any trace | ||
| static int SPAN_LIMIT = 100; |
There was a problem hiding this comment.
aren't we making this configurable from php side?
There was a problem hiding this comment.
done, not this actually though
| } | ||
|
|
||
| // release zend string | ||
| zend_string_release(k); |
There was a problem hiding this comment.
this is releasing k whereas this is releasing the key/value from hashmap OPENCENSUS_G(spans). am i right?
could you add these details as comments preceding those lines?
| } | ||
|
|
||
| function opencensus_trace_remove_span($key): bool { | ||
|
|
There was a problem hiding this comment.
any reason to add it here in daemon functions?
There was a problem hiding this comment.
It doesn't affect anything adding here, but it lets developer know what are the c functions defined and their use
| // checks if spanlimit has reached and if yes flushes the closed spans | ||
| if (Curl::$tracer != null) { | ||
| Curl::$tracer->checkSpanLimit(); | ||
| } | ||
|
|
There was a problem hiding this comment.
this can't be expected out of every handler function and we should think of a better way for it.
| } | ||
| } | ||
|
|
||
| $this->export($closedSpans); |
There was a problem hiding this comment.
can we rename this to capture removal part of it too?
|
Also, add our approach of reducing memory bloat in the readme here, removing this part that I added before. |
| PHP_FE(opencensus_trace_function, arginfo_opencensus_trace_function) | ||
| PHP_FE(opencensus_trace_method, arginfo_opencensus_trace_method) | ||
| PHP_FE(opencensus_trace_list, NULL) | ||
| PHP_FE(opencensus_trace_count, NULL) |
There was a problem hiding this comment.
opencensus_span_count is a better name, but I think you kept it for being consistent with other names. let's leave it.
| } | ||
|
|
||
| // deleting th span assosciated with the given span_id | ||
| if (zend_hash_del(OPENCENSUS_G(spans), k) != SUCCESS) { |
There was a problem hiding this comment.
this deletes the corresponding entry of the span in the hashtable. does it remove the actual span too to be able to release memory actually? I mean how/where span_dtor is getting called from here?
There was a problem hiding this comment.
yes, span_dtor is getting called from here, checked with log and also verified locally by using echo(get_human_readable_size(memory_get_peak_usage(true)));
There was a problem hiding this comment.
When inserting the sapn, we also pass the destructor function,
example -> zend_hash_init(OPENCENSUS_G(spans), 16, NULL, span_dtor, 0);
this function gets called on zend_hash_del
https://www.phpinternalsbook.com/php5/hashtables/hashtable_api.html
| { | ||
| $count = opencensus_trace_count(); | ||
|
|
||
| if ($count > $this->spanLimit) { |
| } | ||
|
|
||
| /* This checks the numbet of spans in memory and if the count is more than the set limit, it exports all | ||
| the closed span present in memory, to free up the memory */ |
There was a problem hiding this comment.
also add why we're exporting and deleting only closed spans.
|
|
||
| /* This checks the numbet of spans in memory and if the count is more than the set limit, it exports all | ||
| the closed span present in memory, to free up the memory */ | ||
| public function checkSpanLimit() |
There was a problem hiding this comment.
let's rename SpanLimit as spanBufferLimit/spanBufferSize or something across the code so that it doesn't mislead as if we're limiting number of spans in a trace.
| // set span limit from options if present | ||
| if (isset($options['span_limit'])){ | ||
| $this->spanLimit = $options['span_limit']; | ||
| } |
| * | ||
| * @param SpanContext|null $initialContext The starting span context. | ||
| * @param null $exporter. | ||
| * @param array $options |
There was a problem hiding this comment.
add about the span_limit/span_buffer_size here
No description provided.