Bugzilla

The problem I am trying to fix is not really OOM problems, I want to reduce the number of reallocating in MakePrefixSet. The reason that this looks like an OOM fix is that while working on this, I found out in most of the cases, our delta algorithm actually increases the memory usage(except phishing table) and then I decided to rework on this(I don't aware the OOM issue you commented while implementing)

(In reply to Gian-Carlo Pascutto [:gcp] from comment #5)

> >I suspect some SafeBrowsing crashes[2] during an update is because we manipulate too many small nsTArray
> 
> >his approach should be able to save a lot of memory reallocation overhead.
> 
> 
> "Memory allocation overhead" is not relevant given the frequency of SafeBrowsing updates, and by itself "manipulate too many small nsTArray" is meaningless - there's nothing against doing so.
> 
> This smells like making some random changes because we're unable to find the root cause of a bug. Maybe it's OOM related, sure, but then that's an even bigger reason to get good data, because the second patch will increase your OOM problems.

You are right, we can't really find the root cause, we did a lot of guess in Bug 1362761, but it doesn't work :(
I don't think this is a OOM issue, I think this is because we don't preset Capacity, append 1.5M records(phishing table) to different nsTArray may trigger a lot of memory reallocation, maybe the underlying memory management went wrong while doing these reallocations(Some crash signatures are platform dependent so I guess this is more likely a lower-level problem) 

The overall idea that I am trying to do here is to spend some time analyzing the whole update flow, to see if there is anything we can improve it, not randomly change things that don't help anything.

The problem I am trying to fix is not really OOM problems, I want to reduce the number of reallocating in MakePrefixSet. The reason that this looks like an OOM fix is that while working on this, I found out in most of the cases, our delta algorithm actually increases the memory usage(except phishing table) and then I decided to rework on this(I don't aware the OOM issue you commented while implementing)

(In reply to Gian-Carlo Pascutto [:gcp] from comment #5)

> >I suspect some SafeBrowsing crashes[2] during an update is because we manipulate too many small nsTArray
> 
> >his approach should be able to save a lot of memory reallocation overhead.
> 
> 
> "Memory allocation overhead" is not relevant given the frequency of SafeBrowsing updates, and by itself "manipulate too many small nsTArray" is meaningless - there's nothing against doing so.
> 
> This smells like making some random changes because we're unable to find the root cause of a bug. Maybe it's OOM related, sure, but then that's an even bigger reason to get good data, because the second patch will increase your OOM problems.

You are right, we can't really find the root cause, we did a lot of guess in Bug 1362761, but it doesn't work :(
I don't think OOM is the reason cause update crash, I think it is because that we don't preset Capacity, append 1.5M records(phishing table) to different nsTArray may trigger a lot of memory reallocation, maybe the underlying memory management went wrong while doing these reallocations(Some crash signatures are platform dependent so I guess this is more likely a lower-level problem) 

The overall idea that I am trying to do here is to spend some time analyzing the whole update flow, to see if there is anything we can improve it, not randomly change things that don't help anything.

The problem I am trying to fix is not really OOM problems, I want to reduce the number of reallocating in MakePrefixSet. The reason that this looks like an OOM fix is that while working on this, I found out in most of the cases, our delta algorithm actually increases the memory usage(except phishing table) and then I decided to rework on this(I don't aware the OOM issue you commented while implementing)

(In reply to Gian-Carlo Pascutto [:gcp] from comment #5)

> >I suspect some SafeBrowsing crashes[2] during an update is because we manipulate too many small nsTArray
> 
> >his approach should be able to save a lot of memory reallocation overhead.
> 
> 
> "Memory allocation overhead" is not relevant given the frequency of SafeBrowsing updates, and by itself "manipulate too many small nsTArray" is meaningless - there's nothing against doing so.
> 
> This smells like making some random changes because we're unable to find the root cause of a bug. Maybe it's OOM related, sure, but then that's an even bigger reason to get good data, because the second patch will increase your OOM problems.

You are right, we can't really find the root cause, we did a lot of guess in Bug 1362761, but it doesn't work :(
I don't think OOM is the reason cause update crash, I think it is because that we don't preset Capacity, append 1.5M records(phishing table for example) to different nsTArray may trigger a lot of memory reallocation, maybe the underlying memory management went wrong while doing these reallocations(Some crash signatures are platform dependent so I guess this is more likely a lower-level problem) 

The overall idea that I am trying to do here is to spend some time analyzing the whole update flow, to see if there is anything we can improve it, not randomly change things that don't help anything.